[musl] possible bug in syslog

[musl] possible bug in syslog

[erny hombre]

Hello,

I think there is a bug in syslog:

This assert fails (in gcc the test is ok):
	#if (LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING) != (LOG_DAEMON|LOG_WARNING))
	#error "LOG_MAKEPRI"
	#endif

This code should produce a log message, but actually it does not:
	setlogmask(LOG_UPTO(LOG_NOTICE));
	syslog(LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE), "LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)");
This works:
	syslog(LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE), "LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)");

---

Maybe these macros in syslog.h are wrong:
	#define	LOG_MAKEPRI(f, p) (((f)<<3)|(p))
	#define LOG_FACMASK 0x3f8
	#define LOG_FAC(p) (((p)&LOG_FACMASK)>>3)

correct version:
	#define	LOG_MAKEPRI(f, p) (((f))|(p))
	#define LOG_FACMASK 0xf8
	#define LOG_FAC(p) (((p)&LOG_FACMASK))

Also a line in syslog.c, __vsyslog() should be changed
from:
	if (!(log_mask & LOG_MASK(priority&7)) || (priority&~0x3ff)) return;
to:
	if (!(log_mask & LOG_MASK(priority&7)) || (priority&~0xff)) return;

----
This is my testprogram to reproduce the error:

#include <syslog.h>

int main(void)
{
#if 1 // test ok
#if (LOG_FAC(LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING)) != LOG_DAEMON)
#error "LOG_FAC"
#endif

#if 0 // test fails
#if (LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING) != (LOG_DAEMON|LOG_WARNING))
#error "LOG_MAKEPRI"
#endif
#endif

	setlogmask(LOG_UPTO(LOG_NOTICE));

	openlog("exampleprog", LOG_CONS | LOG_PID | LOG_NDELAY | LOG_PERROR, LOG_LOCAL1);

	syslog(LOG_DAEMON | LOG_NOTICE, "LOG_DAEMON | LOG_NOTICE");
	syslog(LOG_LOCAL7 | LOG_NOTICE, "LOG_LOCAL7 | LOG_NOTICE");
	syslog(LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE), "LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)");
	// the following call does not write a log message:
	syslog(LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE), "LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)");

	closelog();

	return 0;
}
----

Regards
Erwin

Re: [musl] possible bug in syslog

[Rich Felker]

On Thu, Jun 13, 2024 at 01:44:40PM +0200, erny hombre wrote:
> Hello,
> 
> I think there is a bug in syslog:
> 
> This assert fails (in gcc the test is ok):
> 	#if (LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING) != (LOG_DAEMON|LOG_WARNING))
> 	#error "LOG_MAKEPRI"
> 	#endif

This assertion itself is not valid; the whole reason a LOG_MAKEPRI
macro exists is that it's not guaranteed that the packing be simple
or. But the functionality it goes with is intended  to work.

> This code should produce a log message, but actually it does not:
> 	setlogmask(LOG_UPTO(LOG_NOTICE));
> 	syslog(LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE), "LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)");
> This works:
> 	syslog(LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE), "LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)");
> 
> ---
> 
> Maybe these macros in syslog.h are wrong:
> 	#define	LOG_MAKEPRI(f, p) (((f)<<3)|(p))
> 	#define LOG_FACMASK 0x3f8
> 	#define LOG_FAC(p) (((p)&LOG_FACMASK)>>3)
> 
> correct version:
> 	#define	LOG_MAKEPRI(f, p) (((f))|(p))
> 	#define LOG_FACMASK 0xf8
> 	#define LOG_FAC(p) (((p)&LOG_FACMASK))
> 
> Also a line in syslog.c, __vsyslog() should be changed
> from:
> 	if (!(log_mask & LOG_MASK(priority&7)) || (priority&~0x3ff)) return;
> to:
> 	if (!(log_mask & LOG_MASK(priority&7)) || (priority&~0xff)) return;
> 
> ----
> This is my testprogram to reproduce the error:
> 
> #include <syslog.h>
> 
> int main(void)
> {
> #if 1 // test ok
> #if (LOG_FAC(LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING)) != LOG_DAEMON)
> #error "LOG_FAC"
> #endif
> 
> #if 0 // test fails
> #if (LOG_MAKEPRI(LOG_DAEMON, LOG_WARNING) != (LOG_DAEMON|LOG_WARNING))
> #error "LOG_MAKEPRI"
> #endif
> #endif
> 
> 	setlogmask(LOG_UPTO(LOG_NOTICE));
> 
> 	openlog("exampleprog", LOG_CONS | LOG_PID | LOG_NDELAY | LOG_PERROR, LOG_LOCAL1);
> 
> 	syslog(LOG_DAEMON | LOG_NOTICE, "LOG_DAEMON | LOG_NOTICE");
> 	syslog(LOG_LOCAL7 | LOG_NOTICE, "LOG_LOCAL7 | LOG_NOTICE");
> 	syslog(LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE), "LOG_MAKEPRI(LOG_DAEMON, LOG_NOTICE)");
> 	// the following call does not write a log message:
> 	syslog(LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE), "LOG_MAKEPRI(LOG_LOCAL7, LOG_NOTICE)");
> 
> 	closelog();
> 
> 	return 0;
> }
> ----

If I follow correctly, the bug here is that the nonstandard
functionality of passing a facility to syslog() is not working because
the facility bits are shifted into a mismatching position from where
they're checked when packing them with priority.

As for how to fix it, I assume your patch works, but it doesn't do
anything for existing binaries built with the existing LOG_MAKEPRI
macro, which will silently use the wrong facility.

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.

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.

Thanks for reporting.

Rich

Re: [musl] possible bug in syslog

[Thorsten Glaser]

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

Re: [musl] possible bug in syslog

[Rich Felker]

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

Re: [musl] possible bug in syslog

[Thorsten Glaser]

Rich Felker dixit:

>That seems okay. However I also kinda wonder if LOG_MAKEPRI shouldn't
>just be removed.

Best not to: I’ve seen code defining it as (((fac) << 3) | (pri))
if not defined while grepping, and:

gnu/usr.bin/perl/ext/Sys/Syslog/Syslog.xs:      croak("Your vendor has not defined the Sys::Syslog macro LOG_MAKEPRI");

>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).

It seems that LOG_MAKEPRI and the shift may be needed on OS/2…

>(which TBF is kinda buried; the text in XBD for syslog.h says the
>facilities are for openlog not for openlog and syslog).

Maybe ask on the Open Group mantis for an editorial correction there?

bye,
//mirabilos
-- 
<igli> exceptions: a truly awful implementation of quite a nice idea.
<igli> just about the worst way you could do something like that, afaic.
<igli> it's like anti-design.  <mirabilos> that too… may I quote you on that?
<igli> sure, tho i doubt anyone will listen ;)