NetBSD Problem Report #58906

From www@netbsd.org  Sat Dec 14 16:35:51 2024
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) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 5586E1A923D
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 14 Dec 2024 16:35:51 +0000 (UTC)
Message-Id: <20241214163550.20D961A923F@mollari.NetBSD.org>
Date: Sat, 14 Dec 2024 16:35:50 +0000 (UTC)
From: nightquick@proton.me
Reply-To: nightquick@proton.me
To: gnats-bugs@NetBSD.org
Subject: memmem.c: sync with musl upstream that fixes UB on signed overflow
X-Send-Pr-Version: www-1.0

>Number:         58906
>Category:       misc
>Synopsis:       memmem.c: sync with musl upstream that fixes UB on signed overflow
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    misc-bug-people
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Dec 14 16:40:00 +0000 2024
>Closed-Date:    
>Last-Modified:  Sat Dec 14 16:50:03 +0000 2024
>Originator:     nightquick
>Release:        N/A
>Organization:
>Environment:
N/A
>Description:
Taken from the musl commit message:
unsigned char promotes to int, which can overflow when shifted left by
24 bits or more. this has been reported multiple times but then
forgotten. it's expected to be benign UB, but can trap when built with
explicit overflow catching (ubsan or similar). fix it now.

note that promotion to uint32_t is safe and portable even outside of
the assumptions usually made in musl, since either uint32_t has rank
at least unsigned int, so that no further default promotions happen,
or int is wide enough that the shift can't overflow. this is a
desirable property to have in case someone wants to reuse the code
elsewhere.

Source: https://github.com/bminor/musl/commit/593caa456309714402ca4cb77c3770f4c24da9da

Apparently, there's an unused MIN() macro, which I've removed as well.
>How-To-Repeat:

>Fix:
Change diff.

--- memmem.c.orig	2024-12-14 16:16:40.864410497 +0000
+++ memmem_patch.c	2024-12-14 16:18:46.936629028 +0000
@@ -48,8 +48,8 @@
 static char *threebyte_memmem(const unsigned char *h, size_t k,
     const unsigned char *n)
 {
-	uint32_t nw = n[0] << 24 | n[1] << 16 | n[2] << 8;
-	uint32_t hw = h[0] << 24 | h[1] << 16 | h[2] << 8;
+	uint32_t nw = (uint32_t)n[0] << 24 | n[1] << 16 | n[2] << 8;
+	uint32_t hw = (uint32_t)h[0] << 24 | h[1] << 16 | h[2] << 8;
 	for (h += 3, k -= 3; k; k--, hw = (hw|*h++) << 8)
 		if (hw == nw) return __UNCONST(h - 3);
 	return hw == nw ? __UNCONST(h - 3) : 0;
@@ -58,15 +58,14 @@
 static char *fourbyte_memmem(const unsigned char *h, size_t k,
     const unsigned char *n)
 {
-	uint32_t nw = n[0] << 24 | n[1] << 16 | n[2] << 8 | n[3];
-	uint32_t hw = h[0] << 24 | h[1] << 16 | h[2] << 8 | h[3];
+	uint32_t nw = (uint32_t)n[0] << 24 | n[1] << 16 | n[2] << 8 | n[3];
+	uint32_t hw = (uint32_t)h[0] << 24 | h[1] << 16 | h[2] << 8 | h[3];
 	for (h += 4, k -= 4; k; k--, hw = hw << 8 | *h++)
 		if (hw == nw) return __UNCONST(h - 4);
 	return hw == nw ? __UNCONST(h - 4) : 0;
 }

 #define MAX(a,b) ((a)>(b)?(a):(b))
-#define MIN(a,b) ((a)<(b)?(a):(b))

 #define BITOP(a,b,op) \
  ((a)[(size_t)(b)/(8*sizeof *(a))] op (size_t)1<<((size_t)(b)%(8*sizeof *(a))))

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 14 Dec 2024 16:48:44 +0000
State-Changed-Why:
fixed in HEAD, needs pullup-9 and pullup-10


From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58906 CVS commit: src/common/lib/libc/string
Date: Sat, 14 Dec 2024 16:48:05 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Dec 14 16:48:05 UTC 2024

 Modified Files:
 	src/common/lib/libc/string: memmem.c

 Log Message:
 memmem(3): Nix trailing whitespace.

 No functional change intended.

 Preparation for:

 PR lib/58906: memmem.c: sync with musl upstream that fixes UB on signed overflow


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/common/lib/libc/string/memmem.c

 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/58906 CVS commit: src/common/lib/libc/string
Date: Sat, 14 Dec 2024 16:48:13 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Dec 14 16:48:13 UTC 2024

 Modified Files:
 	src/common/lib/libc/string: memmem.c

 Log Message:
 memmem(3): Avoid undefined behaviour in shift.

 PR lib/58906: memmem.c: sync with musl upstream that fixes UB on
 signed overflow


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.6 src/common/lib/libc/string/memmem.c

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

>Unformatted:

Home	PR Database Search