lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined incorrectly

[david%gutteridge.ca@localhost]

>Number:         57172
>Category:       lib
>Synopsis:       LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 07 01:15:00 +0000 2023
>Originator:     David H. Gutteridge
>Release:        HEAD
>Organization:
TNF
>Environment:
>Description:
In sys/syslog.h, LOG_MAKEPRI is presently defined as:

#define	LOG_MAKEPRI(fac, pri)	(((fac) << 3) | (pri))

This is incorrect, because (excepting LOG_NFACILITIES) each facility
value definition is already shifted, so these get double-shifted if
this macro is used (outside of syslogd(8)'s "mark" facility).

The outlier is:

#define	INTERNAL_MARK	LOG_MAKEPRI(LOG_NFACILITIES, 0)

which ends up being used in syslogd.c, and would be correct, since
LOG_NFACILITIES isn't already shifted in its definition.

Then, on the other hand, also in syslogd.c, line 1557, there is:

pri = LOG_MAKEPRI(LOG_USER, LOG_PRI(pri));

Which would be wrong. (As it is an exceptional case, perhaps this has
never been noticed. I couldn't find any PRs about this topic.)

I see this definition came from historical BSD sources. Comparing with
other projects, I see:
  * FreeBSD addressed this in 1997
  * OpenBSD addressed this in 2010.
  * GNU libc addressed this in 2012
  
FreeBSD removed "<<3" from LOG_MAKEPRI and added it to INTERNAL_MARK.
GNU libc did the same. OpenBSD removed LOG_MAKEPRI entirely, as they
felt fixing it would break other code that presumably worked around the
wrong shift.

Following FreeBSD's lead would give us:

--- syslog.h.orig
+++ syslog.h
@@ -61,12 +61,12 @@
 #define	LOG_PRIMASK	0x07	/* mask to extract priority part (internal) */
 				/* extract priority */
 #define	LOG_PRI(p)	((p) & LOG_PRIMASK)
-#define	LOG_MAKEPRI(fac, pri)	(((fac) << 3) | (pri))
+#define	LOG_MAKEPRI(fac, pri)	((fac) | (pri))
 
 #ifdef SYSLOG_NAMES
 #define	INTERNAL_NOPRI	0x10	/* the "no priority" priority */
 				/* mark "facility" */
-#define	INTERNAL_MARK	LOG_MAKEPRI(LOG_NFACILITIES, 0)
+#define	INTERNAL_MARK	LOG_MAKEPRI((LOG_NFACILITIES<<3), 0)

I looked in the src tree and found no impacts other than to syslogd(8).
(Heimdal carries its own copy of the header with the original BSD "<<3"
definition.) A rudimentary search through pkgsrc patches also turned up
nothing, but I may well have missed something there (or, of course,
workarounds in un-patched code in upstream projects).

I came across this while looking at code in lxqt-globalkeys, which
(reasonably) expects the "((fac) | (pri))" definition found in modern
Linux and FreeBSD.

>How-To-Repeat:

>Fix:
Possibly apply that patch, though I don't know about pkgsrc impacts.