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).
Also as you have moved the falloc() before the redo label, you must
not fdremove() and closef() before goto redo.
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.
bluhm
>
> 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 21 Jan 2017 00:38:18 -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 == 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);
> + 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);
>
> /*
> @@ -360,24 +361,20 @@ redo:
> error = soaccept(so, nam);
> if (!error && name != NULL)
> error = copyaddrout(p, nam, name, namelen, anamelen);
> -
> - if (error) {
> - /* if an error occurred, free the file descriptor */
> - NET_UNLOCK(s);
> - m_freem(nam);
> + 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 != 0) {
> 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);
> }
