NetBSD Problem Report #59054

From www@netbsd.org  Fri Feb  7 16:07:28 2025
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 846C51A923A
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  7 Feb 2025 16:07:28 +0000 (UTC)
Message-Id: <20250207160727.1FEF41A923C@mollari.NetBSD.org>
Date: Fri,  7 Feb 2025 16:07:27 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: cmsg(3) uses malloc(3) unnecessarily
X-Send-Pr-Version: www-1.0

>Number:         59054
>Category:       lib
>Synopsis:       cmsg(3) uses malloc(3) unnecessarily
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          needs-pullups
>Class:          doc-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Feb 07 16:10:00 +0000 2025
>Closed-Date:    
>Last-Modified:  Wed Mar 26 15:19:47 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NetCMSG Alignment
>Environment:
>Description:
For a handful of NetBSD releases over a decade ago, the CMSG_SPACE macro expanded to a non-constant expression, as a hokey workaround for some ABI issue I have long since forgotten about.  That was noncompliant with POSIX, which requires:

`If the argument is an integer constant expression, this macro shall expand to an integer constant expression.'

https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_socket.h.html

Since 2012, we have fixed CMSG_SPACE (and CMSG_LEN) so it is a constant expression (and if the ABI issue is still an issue, well, we can use compat_* to sort it out, if it hasn't already been sorted out):

https://mail-index.netbsd.org/source-changes/2012/01/20/msg030814.html

But while CMSG_SPACE was broken, the man page was updated to dynamically allocate struct cmsghdr objects to avoid variable-length arrays in the example code.  This use of malloc/free is unnecessary, now that CMSG_SPACE is fixed -- we should just allocate statically sized buffers on the stack with it.
>How-To-Repeat:
man cmsg
>Fix:
(filing a PR even though I've already drafted this patch, in order to track pullups)

Index: share/man/man3/CMSG_DATA.3
===================================================================
RCS file: /cvsroot/src/share/man/man3/CMSG_DATA.3,v
retrieving revision 1.3
diff -p -p -u -r1.3 CMSG_DATA.3
--- share/man/man3/CMSG_DATA.3	24 Jan 2015 17:17:01 -0000	1.3
+++ share/man/man3/CMSG_DATA.3	7 Feb 2025 16:06:47 -0000
@@ -98,19 +98,13 @@ struct msghdr	 msg;
 struct cmsghdr	*cmsg;
 /* We use a union to make sure hdr is aligned */
 union {
-	struct cmsghdr hdr;
-	unsigned char	 buf[CMSG_SPACE(sizeof(int))];
-} *cmsgbuf;
+	struct cmsghdr	hdr;
+	unsigned char	buf[CMSG_SPACE(sizeof(int))];
+} cmsgbuf;

-/*
- * We allocate in the heap instead of the stack to avoid C99
- * variable stack allocation, which breaks gcc -fstack-protector.
- */
-if ((cmsgbuf = malloc(sizeof(*cmsgbuf))) == NULL)
-	err(1, "malloc");
 (void)memset(&msg, 0, sizeof(msg));
-msg.msg_control = cmsgbuf->buf;
-msg.msg_controllen = sizeof(cmsgbuf->buf);
+msg.msg_control = cmsgbuf.buf;
+msg.msg_controllen = sizeof(cmsgbuf.buf);

 cmsg = CMSG_FIRSTHDR(&msg);
 cmsg->cmsg_len = CMSG_LEN(sizeof(int));
@@ -120,7 +114,6 @@ cmsg->cmsg_type = SCM_RIGHTS;

 if (sendmsg(s, &msg, 0) == -1)
 	err(1, "sendmsg");
-free(cmsgbuf);
 .Ed
 .Pp
 And an example that receives and decomposes the control message:
@@ -128,15 +121,13 @@ And an example that receives and decompo
 struct msghdr	 msg;
 struct cmsghdr	*cmsg;
 union {
-	struct cmsghdr hdr;
-	unsigned char	 buf[CMSG_SPACE(sizeof(int))];
-} *cmsgbuf;
+	struct cmsghdr	hdr;
+	unsigned char	buf[CMSG_SPACE(sizeof(int))];
+} cmsgbuf;

-if ((cmsgbuf = malloc(sizeof(*cmsgbuf))) == NULL)
-	err(1, "malloc");
 (void)memset(&msg, 0, sizeof(msg));
-msg.msg_control = cmsgbuf->buf;
-msg.msg_controllen = sizeof(cmsgbuf->buf);
+msg.msg_control = cmsgbuf.buf;
+msg.msg_controllen = sizeof(cmsgbuf.buf);

 if (recvmsg(s, &msg, 0) == -1)
 	err(1, "recvmsg");
@@ -151,7 +142,6 @@ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg !=
 		/* Do something with the descriptor. */
 	}
 }
-free(cmsgbuf);
 .Ed
 .Sh SEE ALSO
 .Xr recvmsg 2 ,

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59054 CVS commit: src/share/man/man3
Date: Sat, 8 Feb 2025 13:18:48 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Feb  8 13:18:48 UTC 2025

 Modified Files:
 	src/share/man/man3: CMSG_DATA.3

 Log Message:
 cmsg(3): Don't use malloc in example.

 Since CMSG_SPACE (and CMSG_LEN) is required to expand to an integer
 constant expression by POSIX, and it does in NetBSD as of 2012, there
 is no reason to use malloc here -- we can just allocate the buffer on
 the stack without tripping over variable-length arrays.

 PR lib/59054: cmsg(3) uses malloc(3) unnecessarily


 To generate a diff of this commit:
 cvs rdiff -u -r1.3 -r1.4 src/share/man/man3/CMSG_DATA.3

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59054 CVS commit: src/share/man/man3
Date: Sat, 8 Feb 2025 13:30:42 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Feb  8 13:30:42 UTC 2025

 Modified Files:
 	src/share/man/man3: CMSG_DATA.3

 Log Message:
 cmsg(3): Handle as many descriptors as the sender sends in example.

 Due to a quirk of alignment, sizing the control buffer for
 CMSG_SPACE(sizeof(int)) does not guarantee at most one file
 descriptor fits in the buffer -- so if there is a chance the sender
 may send more than one descriptor, the receiver has to loop over all
 of them to avoid leaks.

 Prompted by:

 PR lib/59054: cmsg(3) uses malloc(3) unnecessarily


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/share/man/man3/CMSG_DATA.3

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59054 CVS commit: src/share/man/man3
Date: Wed, 26 Mar 2025 14:12:16 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Mar 26 14:12:16 UTC 2025

 Modified Files:
 	src/share/man/man3: CMSG_DATA.3

 Log Message:
 cmsg(3): Clarify guarantees about CMSG_SPACE/LEN constancy.

 Add some line breaks while here to break up walls of text so it's
 easier to compare the two paragraphs and see how CMSG_SPACE is meant
 for struct msghdr::msg_controllen and CMSG_LEN is meant for struct
 cmsghdr::cmsg_len.

 PR lib/59054: cmsg(3) uses malloc(3) unnecessarily


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.6 src/share/man/man3/CMSG_DATA.3

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

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 26 Mar 2025 15:19:47 +0000
State-Changed-Why:
fixed in HEAD, needs pullup-9 and pullup-10


>Unformatted:

Home	PR Database Search