Re: [musl] possible bug in syslog

[Rich Felker <dalias@libc.org>]

On Thu, Jun 13, 2024 at 03:20:11PM +0000, Thorsten Glaser wrote:
> Rich Felker dixit:
> 
> >If I follow correctly, the bug here is that the nonstandard
> >functionality of passing a facility to syslog()
> 
> At least on BSD, it is expected to work:
> 
> |   The facility parameter encodes a default facility to be assigned to all
> |   messages that do not have an explicit facility encoded:
> 
> Although it also has…
> 
> | #define LOG_MAKEPRI(fac, pri)   (((fac) << 3) | (pri))
> 
> … the uses are…
> 
> | bin/date/date.c:  syslog(LOG_AUTH | LOG_NOTICE, "date set by %s", pc);
> 
> … and…
> 
> | #define LOG_USER        (1<<3)  /* random user-level messages */
> 
> … so I guess it suffers from the same bug.
> 
> $ ./a.out
> exampleprog[11414]: LOG_DAEMON | LOG_NOTICE
> exampleprog[11414]: LOG_LOCAL7 | LOG_NOTICE
> exampleprog[11414]: LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)
> exampleprog[11414]: syslog: unknown facility/priority: 5c5
> exampleprog[11414]: LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)
> 
> The only users of LOG_MAKEPRI in the entire tree are Sys::Syslog::Syslog
> (Perl XS) though and…
> 
> sys/sys/syslog.h:#define        INTERNAL_MARK   LOG_MAKEPRI(LOG_NFACILITIES, 0)
> sys/sys/syslog.h:       { "mark",       INTERNAL_MARK },        /* INTERNAL */
> 
> … so this is probably fixed more easily than when having
> to keep ABI. LOG_FAC has one more user than Perl, but…
> 
>         /*
>          * Don't allow users to log kernel messages.
>          * NOTE: since LOG_KERN == 0 this will also match
>          * messages with no facility specified.
>          */
>         if (LOG_FAC(pri) == LOG_KERN)
>                 pri = LOG_USER | LOG_PRI(pri);
> 
> … this is also very nicely written to not hit this bug.
> 
> LOG_FACMASK must of course match the next power-of-two-minus-one
> for (LOG_NFACILITIES << 3)… so, yes, 0xF8 is correct for BSD as well.
> 
> What do you think about…
> 
> #define LOG_MAKEPRI(fac,pri)    (((fac) & LOG_FACMASK) | ((pri) & LOG_PRIMASK))
> 
> … however?

That seems okay. However I also kinda wonder if LOG_MAKEPRI shouldn't
just be removed. This would highlight anything that was previously
broken, and there seems to be utterly no reason to use it when the
standard already specifies that you just or (I missed that before).

> With the three changes applied, the test program is happy:
> 
> $ ./a.out
> exampleprog[6837]: LOG_DAEMON | LOG_NOTICE
> exampleprog[6837]: LOG_LOCAL7 | LOG_NOTICE
> exampleprog[6837]: LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)
> exampleprog[6837]: LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)
> 
> >Another option would be to leave the macro definitions alone (i.e.
> >preserve existing ABI) and make syslog accept the packing we
> >inadvertently defined with the 3 empty bits between priority and
> >facility.
> 
> This would not bring the MAKEPRI and the “or” method into
> congruence, and, assuming you actually test that the three
> bits are 0 and at least one higher isn’t, it would still mean
> LOG_UUCP/LOG_USER, LOG_LOCAL0/LOG_MAIL, LOG_NFACILITIES/LOG_DAEMON
> (strictly speaking) confusion.

Indeed, I was wrong about this and missed the standard requirement
(which TBF is kinda buried; the text in XBD for syslog.h says the
facilities are for openlog not for openlog and syslog).

Rich