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

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

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