#3474 device lib: strtoul overflow check does not work

[Martin Kaiser]

strtoul should return ULONG_MAX if the input string contains a number that is too large to fit into an unsigned long.

The strtoul implementation in device/lib/strtoul.c has a check for this
(both ret and oldret are unsigned long int)

  oldret = ret;                                                             
  ret *= base;                                                              
  if (ret < oldret)                                                         
          range_error = true;

This is not sufficient to catch all overflows.

If ret == 0x1dcd6500 and base == 10, base * ret = 0x12A05F200, this is truncated to 0x2A05F200. ret > oldret in this case and we get no range error despite the overflow.

Reproducing this issue is as simple as

char s[] = "5000000000";
unsigned long l = strtoul(s, NULL, 10);
printf("l == %ld\n", l);

l is 705032704, which is incorrect.

Printing the intermediate values in strtoul's for loop shows the overflow

ret 0 (0x0), oldret 0 (0x0)
ret 50 (0x32), oldret 5 (0x5)
ret 500 (0x1f4), oldret 50 (0x32)
ret 5000 (0x1388), oldret 500 (0x1f4)
ret 50000 (0xc350), oldret 5000 (0x1388)
ret 500000 (0x7a120), oldret 50000 (0xc350)
ret 5000000 (0x4c4b40), oldret 500000 (0x7a120)
ret 50000000 (0x2faf080), oldret 5000000 (0x4c4b40)
ret 500000000 (0x1dcd6500), oldret 50000000 (0x2faf080)
ret 705032704 (0x2a05f200), oldret 500000000 (0x1dcd6500)
-> here's the overflow that's not caught by the range_error check

I'm not sure how to fix this properly. The only way I can think of is to use an unsigned long long for ret and oldret. If that's acceptable, I can try to cook up a patch.

Thanks & best regards,
Martin

[Benedikt Freisen]

I'm not sure how to fix this properly. The only way I can think of is to use an unsigned long long for ret and oldret. If that's acceptable, I can try to cook up a patch.

It is unfortunately not really sufficient, because it would not transfer to strtoull, which appears to be missing right now.
The best overflow check I can think of would compare ret to ULONG_MAX / base before the multiplication.
No idea whether there is a way to do it without division.

Incidentally, the current implementation does not yet support C2X's 0b prefix, either.

[Philipp Klaus Krause]

The obvious solution is to use a multiplication that checks for overflow. We recently added support for C2X chk_mul (which does not yet support unsigned long long, so it won't help for strtoull). The current implementation uses (unsigned) long long internally, but chk_mul will likely be migrated to more efficient code paths in the future.
Though the (unmaintained) pic ports don't support (unsigned) long long.

[Philipp Klaus Krause]

Fixed in [r13811]. Except for pic (the current solution uses unsigned long long internally, which pic doesn't support).