Re: [musl] possible bug in syslog

[Thorsten Glaser <tg@mirbsd.de>]

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?

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.

>I'm not sure what we should do since this functionality has never
>worked right, and since it's fairly obscure and nonstandard. Let's
>wait a bit and see if anyone else has opinions on which way to fix it.

Did musl crib the logging framework from a BSD?

In that case I’d go with the suggested fix, as LOG_MAKEPRI probably
is more rarely used. I was just going to do a codesearch on Debian
when I saw the new reply (not in the thread) that…

>Here (https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/syslog.html)
>I found this: Values of the priority argument are formed by OR'ing
>together a severity-level value and an optional facility value.

… indicates that, yes, “or”ing must be possible.

I’ll commit the fix to MirBSD now. Thanks erny for raising.

bye,
//mirabilos
-- 
> Hi, does anyone sell openbsd stickers by themselves and not packaged
> with other products?
No, the only way I've seen them sold is for $40 with a free OpenBSD CD.
	-- Haroon Khalid and Steve Shockley in gmane.os.openbsd.misc