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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 07 01:15:00 +0000 2023
>Closed-Date:    Fri Aug 23 03:03:25 +0000 2024
>Last-Modified:  Sat Aug 24 05:55:01 +0000 2024
>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.

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined
 incorrectly
Date: Fri, 2 Aug 2024 22:41:31 +0000 (UTC)

 I tripped over this same thing recently and made the patch below. It:

 a) Gets rid of LOG_MAKEPRI(). The only user in-tree is syslogd(8).

 b) Simplify INTERNAL_MARK.

 c) Adds a `-k' flag to not translate remote kern.* -> user.* (from
     FreeBSD).

     (src/lib/libc/gen/xsyslog.c forces this on all locally-generated
      kern.* messages. Maybe it shouldn't since syslogd does it anyway.)

 d) Fix syslogd:
     - SIGN_NUM_PRIVALS not defined anywhere.
     - Use the standard idiom to make `priority'.

 -RVP

 ---START patch---
 diff -urN a/src/crypto/external/bsd/heimdal/dist/lib/roken/syslog.hin b/src/crypto/external/bsd/heimdal/dist/lib/roken/syslog.hin
 --- a/src/crypto/external/bsd/heimdal/dist/lib/roken/syslog.hin	2011-04-13 18:15:43.000000000 +0000
 +++ b/src/crypto/external/bsd/heimdal/dist/lib/roken/syslog.hin	2024-07-28 21:53:12.784379077 +0000
 @@ -86,12 +86,11 @@
   #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))

   #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_NFACILITIES << 3)
   typedef struct _code {
   	char	*c_name;
   	int	c_val;
 diff -urN a/src/sys/sys/syslog.h b/src/sys/sys/syslog.h
 --- a/src/sys/sys/syslog.h	2024-07-12 03:31:26.109855531 +0000
 +++ b/src/sys/sys/syslog.h	2024-07-28 21:55:25.744625715 +0000
 @@ -62,12 +62,11 @@
   #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))

   #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_NFACILITIES << 3)
   typedef struct _code {
   	const char	*c_name;
   	int	c_val;
 diff -urN a/src/usr.sbin/syslogd/syslogd.8 b/src/usr.sbin/syslogd/syslogd.8
 --- a/src/usr.sbin/syslogd/syslogd.8	2022-11-08 01:43:09.000000000 +0000
 +++ b/src/usr.sbin/syslogd/syslogd.8	2024-07-28 23:12:21.623389569 +0000
 @@ -37,7 +37,7 @@
   .Nd log systems messages
   .Sh SYNOPSIS
   .Nm
 -.Op Fl nrSsTUvX
 +.Op Fl knrSsTUvX
   .Op Fl B Ar buffer_length
   .Op Fl b Ar bind_address
   .Op Fl d Op Oo Cm \&~ Oc Ns Ar what
 @@ -91,6 +91,15 @@
   Set GID to
   .Ar group
   after the sockets and log files have been opened.
 +.It Fl k
 +Disable the translation of (remote) messages received with facility
 +.Dq kern
 +to facility
 +.Dq user .
 +Usually the
 +.Dq kern
 +facility is reserved for messages read directly from
 +.Pa /dev/klog .
   .It Fl m Ar mark_interval
   Select the number of minutes between ``mark'' messages;
   the default is 20 minutes.
 diff -urN a/src/usr.sbin/syslogd/syslogd.c b/src/usr.sbin/syslogd/syslogd.c
 --- a/src/usr.sbin/syslogd/syslogd.c	2023-10-11 23:22:13.000000000 +0000
 +++ b/src/usr.sbin/syslogd/syslogd.c	2024-07-28 22:54:20.649653887 +0000
 @@ -205,6 +205,7 @@
   				 * this, it will only break some syslog-sign
   				 * configurations (e.g. with SG="0").
   				 */
 +bool	KernXlat = true;	/* translate kern.* -> user.* */
   char	appname[]   = "syslogd";/* the APPNAME for own messages */
   char   *include_pid;		/* include PID in own messages */
   char	include_pid_buf[11];
 @@ -319,7 +320,7 @@
   	/* should we set LC_TIME="C" to ensure correct timestamps&parsing? */
   	(void)setlocale(LC_ALL, "");

 -	while ((ch = getopt(argc, argv, "b:B:d::nsSf:m:o:p:P:ru:g:t:TUvX")) != -1)
 +	while ((ch = getopt(argc, argv, "b:B:d::knsSf:m:o:p:P:ru:g:t:TUvX")) != -1)
   		switch(ch) {
   		case 'b':
   			bindhostname = optarg;
 @@ -360,6 +361,9 @@
   			if (*group == '\0')
   				usage();
   			break;
 +		case 'k':		/* pass-thru (remote) kern.* */
 +			KernXlat = true;
 +			break;
   		case 'm':		/* mark interval */
   			MarkInterval = atoi(optarg) * 60;
   			break;
 @@ -557,7 +561,7 @@
   #if (IETF_NUM_PRIVALUES != (LOG_NFACILITIES<<3))
   	logerror("Warning: system defines %d priority values, but "
   	    "syslog-protocol/syslog-sign specify %d values",
 -	    LOG_NFACILITIES, SIGN_NUM_PRIVALS);
 +	    LOG_NFACILITIES, IETF_NUM_PRIVALUES >> 3);
   #endif

   	/*
 @@ -1549,12 +1553,12 @@
   		pri = DEFUPRI;

   	/*
 -	 * Don't allow users to log kernel messages.
 +	 * Don't (usually) allow users to log kernel messages.
   	 * NOTE: Since LOG_KERN == 0, this will also match
   	 *	 messages with no facility specified.
   	 */
 -	if ((pri & LOG_FACMASK) == LOG_KERN)
 -		pri = LOG_MAKEPRI(LOG_USER, LOG_PRI(pri));
 +	if ((pri & LOG_FACMASK) == LOG_KERN && KernXlat)
 +		pri = LOG_USER | LOG_PRI(pri);

   	if (bsdsyslog) {
   		buffer = printline_bsdsyslog(hname, p, flags, pri);
 ---END patch---

From: "David H. Gutteridge" <gutteridge@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57172 CVS commit: src/usr.sbin/syslogd
Date: Sat, 3 Aug 2024 02:43:37 +0000

 Module Name:	src
 Committed By:	gutteridge
 Date:		Sat Aug  3 02:43:37 UTC 2024

 Modified Files:
 	src/usr.sbin/syslogd: syslogd.c

 Log Message:
 syslogd.c: avoid use of undefined macro (NFCI)

 SIGN_NUM_PRIVALS isn't defined anywhere; fix the block, though it's not
 actually applicable in NetBSD builds. Issue noted by and patch from RVP
 as a peripheral item in PR lib/57172.


 To generate a diff of this commit:
 cvs rdiff -u -r1.141 -r1.142 src/usr.sbin/syslogd/syslogd.c

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

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined
 incorrectly
Date: Sat, 3 Aug 2024 03:04:42 +0000 (UTC)

 On Fri, 2 Aug 2024, RVP wrote:

 > diff -urN a/src/usr.sbin/syslogd/syslogd.c b/src/usr.sbin/syslogd/syslogd.c
 > --- a/src/usr.sbin/syslogd/syslogd.c	2023-10-11 23:22:13.000000000 +0000
 > +++ b/src/usr.sbin/syslogd/syslogd.c	2024-07-28 22:54:20.649653887 +0000
 > @@ -360,6 +361,9 @@
 >   			if (*group == '\0')
 >   				usage();
 >   			break;
 > +		case 'k':		/* pass-thru (remote) kern.* */
 > +			KernXlat = true;
 > +			break;
 >   		case 'm':		/* mark interval */
 >   			MarkInterval = atoi(optarg) * 60;
 >   			break;
 >

 Ah, that should be:

  		KernXlat = false;

 -RVP

From: "David H. Gutteridge" <david@gutteridge.ca>
To: gnats-bugs@netbsd.org, rvp@SDF.ORG
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
Date: Fri, 02 Aug 2024 23:36:55 -0400

 I've got no issue removing LOG_MAKEPRI. It's probably simpler to do
 that and have it reveal any use in pkgsrc, same approach OpenBSD used.

 It would be nice to have input from others, to be sure we're aligned
 on the approach.

 Thanks,

 Dave

From: "David H. Gutteridge" <gutteridge@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57172 CVS commit: src
Date: Wed, 21 Aug 2024 16:30:27 +0000

 Module Name:	src
 Committed By:	gutteridge
 Date:		Wed Aug 21 16:30:27 UTC 2024

 Modified Files:
 	src/sys/sys: syslog.h
 	src/usr.sbin/syslogd: syslogd.c

 Log Message:
 syslog.h & syslogd.c: avoid incorrect facility double-shifting

 As discussed in PR lib/57172, don't double-shift facility values when
 calculating logging contexts. Patch suggested by RVP, an approach also
 consistent with what OpenBSD did, which is to simply remove LOG_MAKEPRI
 and adjust the only place it's used in the tree. (This has the benefit
 of exposing any third-party software that may have also been using the
 incorrect value all this time.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.43 -r1.44 src/sys/sys/syslog.h
 cvs rdiff -u -r1.142 -r1.143 src/usr.sbin/syslogd/syslogd.c

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

From: "David H. Gutteridge" <gutteridge@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57172 CVS commit: src/usr.sbin/syslogd
Date: Wed, 21 Aug 2024 17:13:24 +0000

 Module Name:	src
 Committed By:	gutteridge
 Date:		Wed Aug 21 17:13:24 UTC 2024

 Modified Files:
 	src/usr.sbin/syslogd: syslogd.8 syslogd.c

 Log Message:
 syslogd.8 & syslogd.c: add -k option

 Provide a means of disabling the translation of (remote) messages
 received with facility kern to facility user. Feature equivalent to
 what FreeBSD added years ago, though the code is slightly different
 (a bit easier to follow expressively). Patches from RVP, provided in
 PR lib/57172 (with very minor tweaks by me).


 To generate a diff of this commit:
 cvs rdiff -u -r1.59 -r1.60 src/usr.sbin/syslogd/syslogd.8
 cvs rdiff -u -r1.143 -r1.144 src/usr.sbin/syslogd/syslogd.c

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

From: "David H. Gutteridge" <david@gutteridge.ca>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
Date: Wed, 21 Aug 2024 17:05:23 -0400

 I applied most of this patch set as two commits, to separate the new
 functionality, should someone need to review later.

 I'm not going to change Heimdal, as that's a third-party component with
 an active upstream; the LOG_MAKEPRI definition there is unused, so no
 point adding an additional diff impacting the next time someone imports
 a new version. (There's example software in pkgsrc that's quite the
 same, with that ancient definition that's been copy-pasted, but isn't
 seemingly used anywhere, e.g., mariadb.)

 I will also look into potential breakage in pkgsrc as a result of this
 change, once I have an up to date test environment.

From: "David H. Gutteridge" <david@gutteridge.ca>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined incorrectly
Date: Wed, 21 Aug 2024 20:25:31 -0400

 On Wed, 2024-08-21 at 17:05 -0400, David H. Gutteridge wrote:

 [...]
 > I will also look into potential breakage in pkgsrc as a result of this
 > change, once I have an up to date test environment.

 Apparently there is no breakage in pkgsrc; the only package involved I'd
 already dealt with. From examining OpenBSD's ports, I can see they carry
 a patch for gkrellm that would be relevant, except, the version we have
 in pkgsrc right now is older and doesn't need it. (We have 2.2.10, they
 have 2.3.10.)

State-Changed-From-To: open->closed
State-Changed-By: gutteridge@NetBSD.org
State-Changed-When: Fri, 23 Aug 2024 03:03:25 +0000
State-Changed-Why:
Issue resolved. (No need to pull up to branches, IMO.)

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/57172: LOG_MAKEPRI macro in sys/syslog.h defined
 incorrectly
Date: Sat, 24 Aug 2024 05:54:56 +0000 (UTC)

 On Wed, 21 Aug 2024, gnats-admin@netbsd.org wrote:

 > I applied most of this patch set as two commits, to separate the new
 > functionality, should someone need to review later.
 >

 I see that you also added `-k' to the usage message, which I missed. Thanks!

 -RVP

>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2024 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.