NetBSD Problem Report #57172

From www@netbsd.org  Sat Jan  7 01:10:08 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 2C5E91A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  7 Jan 2023 01:10:08 +0000 (UTC)
Message-Id: <20230107010936.AC6221A923A@mollari.NetBSD.org>
Date: Sat,  7 Jan 2023 01:09:36 +0000 (UTC)
From: david@gutteridge.ca
Reply-To: david@gutteridge.ca
To: gnats-bugs@NetBSD.org
Subject: LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
X-Send-Pr-Version: www-1.0

>Number:         57172
>Category:       lib
>Synopsis:       LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    gutteridge
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 07 01:15:00 +0000 2023
>Last-Modified:  Sat Jan 14 02:23:58 +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.

>Release-Note:

>Audit-Trail:
From: "David H. Gutteridge" <gutteridge@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57172 CVS commit: pkgsrc/x11/lxqt-globalkeys
Date: Sat, 7 Jan 2023 01:26:15 +0000

 Module Name:	pkgsrc
 Committed By:	gutteridge
 Date:		Sat Jan  7 01:26:15 UTC 2023

 Modified Files:
 	pkgsrc/x11/lxqt-globalkeys: Makefile distinfo
 	pkgsrc/x11/lxqt-globalkeys/patches: patch-daemon_core.cpp

 Log Message:
 lxqt-globalkeys: adjust previous LOG_MAKEPRI patch

 This is also actually relevant to NetBSD, which defines it incorrectly
 (PR lib/57172).


 To generate a diff of this commit:
 cvs rdiff -u -r1.21 -r1.22 pkgsrc/x11/lxqt-globalkeys/Makefile
 cvs rdiff -u -r1.11 -r1.12 pkgsrc/x11/lxqt-globalkeys/distinfo
 cvs rdiff -u -r1.3 -r1.4 \
     pkgsrc/x11/lxqt-globalkeys/patches/patch-daemon_core.cpp

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: lib-bug-people->gutteridge
Responsible-Changed-By: gutteridge@NetBSD.org
Responsible-Changed-When: Sat, 14 Jan 2023 02:23:58 +0000
Responsible-Changed-Why:
Take this, so it doesn't get lost.

>Unformatted:

Home	PR Database Search