NetBSD Problem Report #58041

From www@netbsd.org  Fri Mar 15 19:28:53 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))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id F240D1A924E
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 15 Mar 2024 19:28:52 +0000 (UTC)
Message-Id: <20240315192851.2923C1A924F@mollari.NetBSD.org>
Date: Fri, 15 Mar 2024 19:28:51 +0000 (UTC)
From: torreemanuele6@gmail.com
Reply-To: torreemanuele6@gmail.com
To: gnats-bugs@NetBSD.org
Subject: strptime("%s") bug: undefined behaviour in 64-bit; accepts invalid values in 32-bit
X-Send-Pr-Version: www-1.0

>Number:         58041
>Category:       lib
>Synopsis:       strptime("%s") bug: undefined behaviour in 64-bit; accepts invalid values in 32-bit
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    riastradh
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Mar 15 19:30:01 +0000 2024
>Closed-Date:    
>Last-Modified:  Tue Apr 16 18:35:01 +0000 2024
>Originator:     emanuele6 (Emanuele Torre)
>Release:        src/lib/libc/time/strptime.c revision 1.63
>Organization:
jq
>Environment:
The version bundled in jq in builds for targets that don't have strptime
>Description:
The `} while ((sse * 10 <= TIME_MAX) &&' at line 366 of strptime.c is
always true because `sse * 10' is `time_t' (signed), and `TIME_MAX' is
its maximum value.

This makes the code, if the check is not optimised out, trigger a signed
integer overflow (undefined behaviour in standard C) for inputs to %s
that start with a sequence of characters between "922337203685477581"
and "999999999999999999" (inclusive, 64-bit time_t).

Additionally, in builds where there is `#define TIME_MAX INT32_MAX', and
`time_t' is `int32_t', if we assume that signed integer overflow works
like unsigned integer overflow, this makes %s accept, for example,
"9999999999" as a valid timestamp equivalent to
"1410065398" (Sat 20 Nov 17:46:39 UTC) instead of rejecting it.
This is thanks to the fact that UINT32_MAX and INT32_MAX (unlike their
64-bit counterparts) have the same number of digits in decimal, so
`rulim /= 10' can't reject the overflow in time.

>How-To-Repeat:

>Fix:
jq fix ported to src/lib/libc/time/strptime.c revision 1.63

--- strptime.c	2024-03-15 19:56:06.096430188 +0100
+++ strptime-patch.c	2024-03-15 19:57:22.722593741 +0100
@@ -351,7 +351,8 @@
 #endif
 		case 's':	/* seconds since the epoch */
 			{
-				time_t sse = 0;
+				time_t tsse;
+				uint64_t sse = 0;
 				uint64_t rulim = TIME_MAX;

 				if (*bp < '0' || *bp > '9') {
@@ -366,12 +367,13 @@
 				} while ((sse * 10 <= TIME_MAX) &&
 					 rulim && *bp >= '0' && *bp <= '9');

-				if (sse < 0 || (uint64_t)sse > TIME_MAX) {
+				if (sse > TIME_MAX) {
 					bp = NULL;
 					continue;
 				}

-				if (localtime_r(&sse, tm) == NULL)
+				tsse = sse;
+				if (localtime_r(&tsse, tm) == NULL)
 					bp = NULL;
 				else
 					state |= S_YDAY | S_WDAY |

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: lib-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Sat, 16 Mar 2024 00:08:05 +0000
Responsible-Changed-Why:
take


State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 16 Mar 2024 00:08:05 +0000
State-Changed-Why:
tests added, UB fixed, need some pullups now


From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58041 CVS commit: src/tests/lib/libc/time
Date: Sat, 16 Mar 2024 00:06:37 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Mar 16 00:06:37 UTC 2024

 Modified Files:
 	src/tests/lib/libc/time: t_strptime.c

 Log Message:
 strptime(3): Exercise some edge cases in the automatic tests.

 Unfortunately, we can't quite use strptime as a black box to detect
 the cases that triggered undefined behaviour, because strptime just
 fails in that case anyway since the number that would go in .tm_year
 is far out of the representable range.

 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.15 -r1.16 src/tests/lib/libc/time/t_strptime.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/58041 CVS commit: src/lib/libc/time
Date: Sat, 16 Mar 2024 00:06:46 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Mar 16 00:06:46 UTC 2024

 Modified Files:
 	src/lib/libc/time: strptime.c

 Log Message:
 strptime(3): Avoid arithmetic overflow.

 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.63 -r1.64 src/lib/libc/time/strptime.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/58041 CVS commit: src/lib/libc/time
Date: Sat, 16 Mar 2024 00:16:21 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sat Mar 16 00:16:21 UTC 2024

 Modified Files:
 	src/lib/libc/time: strptime.c

 Log Message:
 strptime(3): Reduce unnecessary indentation.

 Post-fix tidying.

 No functional change intended.

 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.64 -r1.65 src/lib/libc/time/strptime.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: Emanuele Torre <torreemanuele6@gmail.com>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: lib/58041: strptime("%s") bug: undefined behaviour in 64-bit; accepts invalid values in 32-bit
Date: Sat, 16 Mar 2024 00:19:30 +0000

 Thanks for the report!

 > This makes the code, if the check is not optimised out, trigger a signed
 > integer overflow (undefined behaviour in standard C) for inputs to %s
 > that start with a sequence of characters between "922337203685477581"
 > and "999999999999999999" (inclusive, 64-bit time_t).

 Yep, that looks like undefined behaviour.  I added some automatic
 tests to exercise this case -- although you can't verify from the
 outside if it's wrong or not, because even if strptime parses it
 correctly, the time is outside the range that can be represented by
 struct tm, so strptime needs to fail either way.

 > Additionally, in builds where there is `#define TIME_MAX INT32_MAX', and
 > `time_t' is `int32_t', if we assume that signed integer overflow works
 > like unsigned integer overflow, this makes %s accept, for example,
 > "9999999999" as a valid timestamp equivalent to
 > "1410065398" (Sat 20 Nov 17:46:39 UTC) instead of rejecting it.
 > This is thanks to the fact that UINT32_MAX and INT32_MAX (unlike their
 > 64-bit counterparts) have the same number of digits in decimal, so
 > `rulim /= 10' can't reject the overflow in time.

 In NetBSD, this is not an issue because time_t is always 64-bit and
 has been since netbsd-6.  (We've been working on getting ready for
 Y2.038k for many years now!)

 However, to make it more portable and easier to understand, I've
 rewritten the logic in a way that uses a simpler idiom to reliably
 avoid overflow no matter what integer type time_t is.  (Porting to
 systems with floating-point time_t left as an exercise for the
 reader!)

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sun, 17 Mar 2024 02:39:35 +0000
State-Changed-Why:
pullup-10 #638
tests don't pass on netbsd-9, to be investigated
haven't yet tried netbsd-8


From: Emanuele Torre <torreemanuele6@gmail.com>
To: gnats-bugs@netbsd.org
Cc: riastradh@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: lib/58041: strptime("%s") bug: undefined behaviour in 64-bit;
 accepts invalid values in 32-bit
Date: Mon, 18 Mar 2024 16:30:53 +0100

 On Sat, Mar 16, 2024 at 12:20:03AM +0000, Taylor R Campbell wrote:
 >  Thanks for the report!
 >  
 >  > This makes the code, if the check is not optimised out, trigger a signed
 >  > integer overflow (undefined behaviour in standard C) for inputs to %s
 >  > that start with a sequence of characters between "922337203685477581"
 >  > and "999999999999999999" (inclusive, 64-bit time_t).
 >  
 >  Yep, that looks like undefined behaviour.  I added some automatic
 >  tests to exercise this case -- although you can't verify from the
 >  outside if it's wrong or not, because even if strptime parses it
 >  correctly, the time is outside the range that can be represented by
 >  struct tm, so strptime needs to fail either way.
 >  
 >  > Additionally, in builds where there is `#define TIME_MAX INT32_MAX', and
 >  > `time_t' is `int32_t', if we assume that signed integer overflow works
 >  > like unsigned integer overflow, this makes %s accept, for example,
 >  > "9999999999" as a valid timestamp equivalent to
 >  > "1410065398" (Sat 20 Nov 17:46:39 UTC) instead of rejecting it.
 >  > This is thanks to the fact that UINT32_MAX and INT32_MAX (unlike their
 >  > 64-bit counterparts) have the same number of digits in decimal, so
 >  > `rulim /= 10' can't reject the overflow in time.
 >  
 >  In NetBSD, this is not an issue because time_t is always 64-bit and
 >  has been since netbsd-6.  (We've been working on getting ready for
 >  Y2.038k for many years now!)
 >  

 \o/

 >  However, to make it more portable and easier to understand, I've
 >  rewritten the logic in a way that uses a simpler idiom to reliably
 >  avoid overflow no matter what integer type time_t is.  (Porting to
 >  systems with floating-point time_t left as an exercise for the
 >  reader!)

 There is a problem with the rewrite

   if (sse > TIME_MAX - d) {

 There (TIME_MAX - d) is unsigned (because d is unsigned int), while sse
 is signed (time_t), and that makes compilers trigger a -Wsign-compare
 warning.

 Also, that code, following C integer promotion rules, is ambiguous
 depending on the type of time_t:

 If time_t is int64_t, it is equivalent to

   if (sse > (int64_t)(TIME_MAX - d)) {

 While, if time_t is int32_t, it is equivalent to

   if ((unsigned)sse > TIME_MAX - d) {

 Either interpretation should be fine in this case, but casting d to
 time_t to prevent the warning would be nice.

 Another alternative, and cleaner fix is to just declare d as int or
 time_t instead of unsigned.

 o/
  emanuele6

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58041 CVS commit: src/lib/libc/time
Date: Mon, 18 Mar 2024 16:15:24 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Mar 18 16:15:24 UTC 2024

 Modified Files:
 	src/lib/libc/time: strptime.c

 Log Message:
 strptime(3): Declare digit d as time_t.

 This doesn't make a semantic difference -- d can only take on the ten
 values {0,1,2,3,4,5,6,7,8,9}, and the arithmetic with it later all
 comes out the same whether the type is unsigned or time_t, even if
 time_t were int32_t instead of int64_t.

 But it pacifies overzealous compilers used by downstream users of
 this code.  And while it's silly to use a much wider type (64-bit
 signed) than is needed here to store a single digit, it doesn't
 really hurt either (32-bit unsigned is much larger than needed too).

 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.65 -r1.66 src/lib/libc/time/strptime.c

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

From: Emanuele Torre <torreemanuele6@gmail.com>
To: gnats-bugs@netbsd.org
Cc: riastradh@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/58041 CVS commit: src/lib/libc/time
Date: Mon, 18 Mar 2024 17:27:43 +0100

 On Mon, Mar 18, 2024 at 04:20:02PM +0000, Taylor R Campbell wrote:
 >  But it pacifies overzealous compilers used by downstream users of
 >  this code.  And while it's silly to use a much wider type (64-bit
 >  signed) than is needed here to store a single digit, it doesn't
 >  really hurt either (32-bit unsigned is much larger than needed too).

 As long as it is a type that has the same signedness as time_t, and can
 hold 0-9, it's fine. :-)

 o/
  emanuele6

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58041 CVS commit: [netbsd-10] src
Date: Mon, 25 Mar 2024 14:43:30 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Mar 25 14:43:30 UTC 2024

 Modified Files:
 	src/lib/libc/time [netbsd-10]: strptime.c
 	src/tests/lib/libc/time [netbsd-10]: t_strptime.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #638):

 	lib/libc/time/strptime.c: revision 1.64
 	lib/libc/time/strptime.c: revision 1.65
 	tests/lib/libc/time/t_strptime.c: revision 1.16

 strptime(3): Exercise some edge cases in the automatic tests.

 Unfortunately, we can't quite use strptime as a black box to detect
 the cases that triggered undefined behaviour, because strptime just
 fails in that case anyway since the number that would go in .tm_year
 is far out of the representable range.
 PR lib/58041

 strptime(3): Avoid arithmetic overflow.
 PR lib/58041

 strptime(3): Reduce unnecessary indentation.
 Post-fix tidying.
 No functional change intended.
 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.63 -r1.63.6.1 src/lib/libc/time/strptime.c
 cvs rdiff -u -r1.15 -r1.15.12.1 src/tests/lib/libc/time/t_strptime.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@NetBSD.org, netbsd-bugs@NetBSD.org, torreemanuele6@gmail.com
Cc: 
Subject: Re: PR/58041 CVS commit: src/lib/libc/time
Date: Mon, 25 Mar 2024 18:11:31 +0000

 - submitted the time_t d change as pullup-10 #648
 - still need to figure out why tests aren't passing on netbsd-9, help
   welcome by anyone who wants to take a look at this

State-Changed-From-To: pending-pullups->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 05 Apr 2024 03:09:36 +0000
State-Changed-Why:
still needs pullup-9, but the tests failed when I last tried


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58041 CVS commit: [netbsd-10] src/lib/libc/time
Date: Tue, 16 Apr 2024 18:33:47 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Apr 16 18:33:47 UTC 2024

 Modified Files:
 	src/lib/libc/time [netbsd-10]: strptime.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #648):

 	lib/libc/time/strptime.c: revision 1.66

 strptime(3): Declare digit d as time_t.

 This doesn't make a semantic difference -- d can only take on the ten
 values {0,1,2,3,4,5,6,7,8,9}, and the arithmetic with it later all
 comes out the same whether the type is unsigned or time_t, even if
 time_t were int32_t instead of int64_t.

 But it pacifies overzealous compilers used by downstream users of
 this code.  And while it's silly to use a much wider type (64-bit
 signed) than is needed here to store a single digit, it doesn't
 really hurt either (32-bit unsigned is much larger than needed too).

 PR lib/58041


 To generate a diff of this commit:
 cvs rdiff -u -r1.63.6.1 -r1.63.6.2 src/lib/libc/time/strptime.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