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:
(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.