[musl] Increase sendmsg internal buffer to match kernel SCM_MAX_FD limit

[musl] Increase sendmsg internal buffer to match kernel SCM_MAX_FD limit

[Colin Cross]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

I came across a test at
https://cs.android.com/android/platform/superproject/+/master:frameworks/native/libs/binder/tests/binderRpcTest.cpp;l=954;drc=68a556190553a4060babf4a4e5cb1bb16ae61ab2
that verifies that some fd passing code can handle passing SCM_MAX_FD
fds through a unix socket.  SCM_MAX_FD is an arbitrary 253 fd limit
imposed by the kernel since 2.6.38 (before that it was 255).  An
SCM_RIGHTS ancillary message contiang 253 fds is only slightly larger
than the existing 1024 byte internal buffer in sendmsg, so this patch
slightly increases the arbitrary limit in musl to match an arbitrary
limit in the kernel.

[-- Attachment #2: 0001-Increase-sendmsg-internal-buffer-to-support-SCM_MAX_.patch --]
[-- Type: text/x-patch, Size: 1648 bytes --]

From 4a9c1a5b14fddd3924561e9cc5d126111ea881c4 Mon Sep 17 00:00:00 2001
From: Colin Cross <ccross@android.com>
Date: Thu, 9 Feb 2023 14:50:49 -0800
Subject: [PATCH] Increase sendmsg internal buffer to support SCM_MAX_FD

The kernel defines a limit on the number of fds that can be passed
through an SCM_RIGHTS ancillary message as SCM_MAX_FD.  The value
has been 253 since kernel 2.6.38 (before that it was 255).  On x86_64,
and SCM_RIGHTS ancillary message with 253 fds requires 1032 bytes,
slightly more than the current 1024 byte internal buffer in sendmsg.
1024 is an arbitrary size, so increase it to match the the arbitrary
size limit in the kernel.  This fixes tests that are verifying they
support up to SCM_MAX_FD fds.
---
 src/network/sendmsg.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/network/sendmsg.c b/src/network/sendmsg.c
index 80cc5f41..b5ce6629 100644
--- a/src/network/sendmsg.c
+++ b/src/network/sendmsg.c
@@ -8,13 +8,16 @@ ssize_t sendmsg(int fd, const struct msghdr *msg, int flags)
 {
 #if LONG_MAX > INT_MAX
 	struct msghdr h;
-	struct cmsghdr chbuf[1024/sizeof(struct cmsghdr)+1], *c;
+	/* Kernels since 2.6.38 set SCM_MAX_FD to 253, allocate enough
+	 * space to support an SCM_RIGHTS ancillary message with 253 fds. */
+	const size_t chbufsize = CMSG_SPACE(253*sizeof(int));
+	struct cmsghdr chbuf[chbufsize/sizeof(struct cmsghdr)+1], *c;
 	if (msg) {
 		h = *msg;
 		h.__pad1 = h.__pad2 = 0;
 		msg = &h;
 		if (h.msg_controllen) {
-			if (h.msg_controllen > 1024) {
+			if (h.msg_controllen > chbufsize) {
 				errno = ENOMEM;
 				return -1;
 			}
-- 
2.39.1.581.gbfd45094c4-goog

Re: [musl] Increase sendmsg internal buffer to match kernel SCM_MAX_FD limit

[Rich Felker]

On Thu, Feb 09, 2023 at 03:08:50PM -0800, Colin Cross wrote:
> I came across a test at
> https://cs.android.com/android/platform/superproject/+/master:frameworks/native/libs/binder/tests/binderRpcTest.cpp;l=954;drc=68a556190553a4060babf4a4e5cb1bb16ae61ab2
> that verifies that some fd passing code can handle passing SCM_MAX_FD
> fds through a unix socket.  SCM_MAX_FD is an arbitrary 253 fd limit
> imposed by the kernel since 2.6.38 (before that it was 255).  An
> SCM_RIGHTS ancillary message contiang 253 fds is only slightly larger
> than the existing 1024 byte internal buffer in sendmsg, so this patch
> slightly increases the arbitrary limit in musl to match an arbitrary
> limit in the kernel.

> From 4a9c1a5b14fddd3924561e9cc5d126111ea881c4 Mon Sep 17 00:00:00 2001
> From: Colin Cross <ccross@android.com>
> Date: Thu, 9 Feb 2023 14:50:49 -0800
> Subject: [PATCH] Increase sendmsg internal buffer to support SCM_MAX_FD
> 
> The kernel defines a limit on the number of fds that can be passed
> through an SCM_RIGHTS ancillary message as SCM_MAX_FD.  The value
> has been 253 since kernel 2.6.38 (before that it was 255).  On x86_64,
> and SCM_RIGHTS ancillary message with 253 fds requires 1032 bytes,
> slightly more than the current 1024 byte internal buffer in sendmsg.
> 1024 is an arbitrary size, so increase it to match the the arbitrary
> size limit in the kernel.  This fixes tests that are verifying they
> support up to SCM_MAX_FD fds.
> ---
>  src/network/sendmsg.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network/sendmsg.c b/src/network/sendmsg.c
> index 80cc5f41..b5ce6629 100644
> --- a/src/network/sendmsg.c
> +++ b/src/network/sendmsg.c
> @@ -8,13 +8,16 @@ ssize_t sendmsg(int fd, const struct msghdr *msg, int flags)
>  {
>  #if LONG_MAX > INT_MAX
>  	struct msghdr h;
> -	struct cmsghdr chbuf[1024/sizeof(struct cmsghdr)+1], *c;
> +	/* Kernels since 2.6.38 set SCM_MAX_FD to 253, allocate enough
> +	 * space to support an SCM_RIGHTS ancillary message with 253 fds. */
> +	const size_t chbufsize = CMSG_SPACE(253*sizeof(int));
> +	struct cmsghdr chbuf[chbufsize/sizeof(struct cmsghdr)+1], *c;
>  	if (msg) {
>  		h = *msg;
>  		h.__pad1 = h.__pad2 = 0;
>  		msg = &h;
>  		if (h.msg_controllen) {
> -			if (h.msg_controllen > 1024) {
> +			if (h.msg_controllen > chbufsize) {
>  				errno = ENOMEM;
>  				return -1;
>  			}
> -- 
> 2.39.1.581.gbfd45094c4-goog
> 

The concept of this seems fine, but if the limit was previously 255 on
supported kernel versions, why stop at 253? It doesn't really cost
anything to go up large enough that the 255 would work too.

As a technical detail, I'd probably also just put the full size
expression in the [], then use sizeof when you need it later. As
written, this change makes chbuf[] formally a VLA. The compiler
probably optimizes it to the same code as if it wasn't a VLA, but
there's no good reason for it to be a VLA.

Rich

Re: [musl] Increase sendmsg internal buffer to match kernel SCM_MAX_FD limit

[Colin Cross]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Thu, Feb 9, 2023 at 4:26 PM Rich Felker <dalias@libc.org> wrote:
> The concept of this seems fine, but if the limit was previously 255 on
> supported kernel versions, why stop at 253? It doesn't really cost
> anything to go up large enough that the 255 would work too.

I can use 255.

> As a technical detail, I'd probably also just put the full size
> expression in the [], then use sizeof when you need it later. As
> written, this change makes chbuf[] formally a VLA. The compiler
> probably optimizes it to the same code as if it wasn't a VLA, but
> there's no good reason for it to be a VLA.

Using sizeof with the original buffer would have effectively fixed
this issue, the extra "+1" in the number of chbuf elements adds 16
extra bytes to the buffer on x86_64, which would have been sufficient
to hold the extra fds.  It might as well be explicit though so it
doesn't get reduced in the future.

Attached v2.

[-- Attachment #2: 0001-Increase-sendmsg-internal-buffer-to-support-SCM_MAX_.patch --]
[-- Type: text/x-patch, Size: 1649 bytes --]

From f80a5b0ed0a00b3946e2ee85b52dbb537c2fe985 Mon Sep 17 00:00:00 2001
From: Colin Cross <ccross@android.com>
Date: Thu, 9 Feb 2023 14:50:49 -0800
Subject: [PATCH] Increase sendmsg internal buffer to support SCM_MAX_FD

The kernel defines a limit on the number of fds that can be passed
through an SCM_RIGHTS ancillary message as SCM_MAX_FD.  The value
was 255 before kernel 2.6.38 (after that it is 253), and an SCM_RIGHTS
ancillary message with 255 fds requires 1040 bytes,
slightly more than the current 1024 byte internal buffer in sendmsg.
1024 is an arbitrary size, so increase it to match the the arbitrary
size limit in the kernel.  This fixes tests that are verifying they
support up to SCM_MAX_FD fds.
---
 src/network/sendmsg.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/network/sendmsg.c b/src/network/sendmsg.c
index 80cc5f41..dad3814f 100644
--- a/src/network/sendmsg.c
+++ b/src/network/sendmsg.c
@@ -8,13 +8,16 @@ ssize_t sendmsg(int fd, const struct msghdr *msg, int flags)
 {
 #if LONG_MAX > INT_MAX
 	struct msghdr h;
-	struct cmsghdr chbuf[1024/sizeof(struct cmsghdr)+1], *c;
+	/* Kernels before 2.6.38 set SCM_MAX_FD to 255, allocate enough
+	 * space to support an SCM_RIGHTS ancillary message with 255 fds.
+	 * Kernels since 2.6.38 set SCM_MAX_FD to 253. */
+	struct cmsghdr chbuf[CMSG_SPACE(255*sizeof(int))/sizeof(struct cmsghdr)+1], *c;
 	if (msg) {
 		h = *msg;
 		h.__pad1 = h.__pad2 = 0;
 		msg = &h;
 		if (h.msg_controllen) {
-			if (h.msg_controllen > 1024) {
+			if (h.msg_controllen > sizeof(chbuf)) {
 				errno = ENOMEM;
 				return -1;
 			}
-- 
2.39.1.581.gbfd45094c4-goog