svn commit: r316874 - head/sys/kern

[Peter Wemm]

On Friday, April 14, 2017 08:13:52 PM Ngie Cooper wrote:
> > On Apr 14, 2017, at 20:12, Peter Wemm <peter at wemm.org> wrote:
> > 
> > On Friday, April 14, 2017 07:36:55 PM Peter Wemm wrote:
> >> On Friday, April 14, 2017 02:14:16 PM Ngie Cooper wrote:
> >>>> On Apr 14, 2017, at 14:10, Maxim Sobolev <sobomax at sippysoft.com> wrote:
> >>>> 
> >>>> Peter, Ngie,
> >>>> 
> >>>> Looks like out of that refactoring came a logical bug that is present
> >>>> in
> >>>> the head, which causes syslod to first to shutdown the socket for
> >>>> reading
> >>>> and then try to select/recv on it (which is somewhat stupid). And that
> >>>> issue has been masked by shutdown() on datagram socket becoming
> >>>> effectively a NOP in 11 & head 20 months ago. It only affects head
> >>>> though, 11-stable still has the old code which does not include that
> >>>> half-closed socket into the select list. Attached patch is expected to
> >>>> fix head, Peter, it would be nice if you can give it a try (restoring
> >>>> latest changes into uipc_sockets.c) and let me know if it helps.
> >>>> 
> >>>> Thanks!
> >>> 
> >>> CCing hrs@ for input as he did the refactoring.
> >>> Thanks!
> >>> -Ngie
> >>> 
> >>> PS LGTM with the change. Will wait for feedback from wemm at .
> >> 
> >> This is definitely not working.  I get ENOSPC  and listen queue overflows
> >> on /var/run/logpriv now.
> >> 
> >> Grabbing an old 10.3 /usr/sbin/syslogd and placing it on the top of the
> >> 12.x one worked fine, aside from the include statements.
> > 
> > This can't be right:
> >                 if (SecureMode || res->ai_family == AF_LOCAL) {
> >                 
> >                        /* Forbid communication in secure mode. */
> >                        if (shutdown(s, SHUT_RD) < 0 &&
> >                        
> >                            errno != ENOTCONN) {
> >                            
> >                                logerror("shutdown");
> >                                if (!Debug)
> >                                
> >                                        die(0);
> >                        
> >                        }
> >                        dprintf("listening on socket\n");
> >                        sl_recv = NULL;
> >                   
> >                   }
> > 
> > This appears to disable unix domain sockets like /var/run/log and
> > /var/run/logpriv.
> 
> ACK. This looks like a fun bug.

> -Ngie

I suspect it's meant to be "if (SecureMode && res->ai_family != AF_LOCAL) {" 
as a simple logic inversion error of another line earlier.  However there's an 
awful lot of strange things in this code.

1) listen(s, 5) - on datagram sockets.
2) dprintf("shutdown") in code regardless of whether the shutdown is going to 
happen.
3) dprintf("listening on socket") in code that only happens when we're NOT 
going to listen.
4) dprintf("sending on socket") in the code path when we're going to listen.
5) shutdown on all unix domain sockets, regardless of securemode..

This code block makes my head spin.

-- 
Peter Wemm - peter at wemm.org; peter at FreeBSD.org; peter at yahoo-inc.com; KI6FJV
UTF-8: for when a ' or ... just won\342\200\231t do\342\200\246
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20170414/590bbe4c/attachment.sig>