Re: lib/58041: strptime("%s") bug: undefined behaviour in 64-bit; accepts invalid values in 32-bit

[Emanuele Torre <torreemanuele6%gmail.com@localhost>]

The following reply was made to PR lib/58041; it has been noted by GNATS.

From: Emanuele Torre <torreemanuele6%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: riastradh%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
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