On 24/01/17(Tue) 00:51, Alexander Bluhm wrote:
> On Sat, Jan 21, 2017 at 10:46:42AM +1000, Martin Pieuchot wrote:
> > Here's another way to fix the problem, call falloc() before grabbing the
> > NET_LOCK().
> >
> > Comments?
>
> There a two bugs in the "goto redo" block that is not part of the
> diff. You could do a m_freem(nam), then goto redo, get an error
> and goto out. This will result in a double m_freem(nam).
Ok, I include this fix.
> Also as you have moved the falloc() before the redo label, you must
> not fdremove() and closef() before goto redo.
Nice catch.
> You you use only if (error) and if (!error) and not mix it with
> (error != 0) and (error == 0)? That makes checking the error paths
> easier.
Fine.
Updated diff, thanks for your review.
Index: kern/uipc_syscalls.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.144
diff -u -p -r1.144 uipc_syscalls.c
--- kern/uipc_syscalls.c 29 Dec 2016 12:12:43 -0000 1.144
+++ kern/uipc_syscalls.c 23 Jan 2017 23:59:34 -0000
@@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
{
struct filedesc *fdp = p->p_fd;
struct file *fp, *headfp;
- struct mbuf *nam;
+ struct mbuf *nam = NULL;
socklen_t namelen;
int error, s, tmpfd;
struct socket *head, *so;
@@ -277,19 +277,35 @@ doaccept(struct proc *p, int sock, struc
return (error);
headfp = fp;
+ head = headfp->f_data;
+
+ fdplock(fdp);
+ error = falloc(p, &fp, &tmpfd);
+ if (!error && (flags & SOCK_CLOEXEC))
+ fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
+ fdpunlock(fdp);
+ if (error) {
+ /*
+ * Probably ran out of file descriptors. Wakeup
+ * so some other process might have a chance at it.
+ */
+ wakeup_one(&head->so_timeo);
+ FRELE(headfp, p);
+ return (error);
+ }
+
redo:
NET_LOCK(s);
- head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
error = EINVAL;
- goto bad;
+ goto out;
}
if ((head->so_state & SS_NBIO) && head->so_qlen == 0) {
if (head->so_state & SS_CANTRCVMORE)
error = ECONNABORTED;
else
error = EWOULDBLOCK;
- goto bad;
+ goto out;
}
while (head->so_qlen == 0 && head->so_error == 0) {
if (head->so_state & SS_CANTRCVMORE) {
@@ -298,34 +314,19 @@ redo:
}
error = tsleep(&head->so_timeo, PSOCK | PCATCH,
"netcon", 0);
- if (error) {
- goto bad;
- }
+ if (error)
+ goto out;
}
if (head->so_error) {
error = head->so_error;
head->so_error = 0;
- goto bad;
+ goto out;
}
/* Figure out whether the new socket should be non-blocking. */
nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
- fdplock(fdp);
- error = falloc(p, &fp, &tmpfd);
- if (error == 0 && (flags & SOCK_CLOEXEC))
- fdp->fd_ofileflags[tmpfd] |= UF_EXCLOSE;
- fdpunlock(fdp);
- if (error != 0) {
- /*
- * Probably ran out of file descriptors. Wakeup
- * so some other process might have a chance at it.
- */
- wakeup_one(&head->so_timeo);
- goto bad;
- }
-
nam = m_get(M_WAIT, MT_SONAME);
/*
@@ -336,10 +337,7 @@ redo:
if (head->so_qlen == 0) {
NET_UNLOCK(s);
m_freem(nam);
- fdplock(fdp);
- fdremove(fdp, tmpfd);
- closef(fp, p);
- fdpunlock(fdp);
+ nam = NULL;
goto redo;
}
@@ -360,24 +358,20 @@ redo:
error = soaccept(so, nam);
if (!error && name != NULL)
error = copyaddrout(p, nam, name, namelen, anamelen);
-
+ if (!error) {
+ (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
+ FILE_SET_MATURE(fp, p);
+ *retval = tmpfd;
+ }
+out:
+ NET_UNLOCK(s);
+ m_freem(nam);
if (error) {
- /* if an error occurred, free the file descriptor */
- NET_UNLOCK(s);
- m_freem(nam);
fdplock(fdp);
fdremove(fdp, tmpfd);
closef(fp, p);
fdpunlock(fdp);
- goto out;
}
- (*fp->f_ops->fo_ioctl)(fp, FIONBIO, (caddr_t)&nflag, p);
- FILE_SET_MATURE(fp, p);
- *retval = tmpfd;
- m_freem(nam);
-bad:
- NET_UNLOCK(s);
-out:
FRELE(headfp, p);
return (error);
}
