Summary

string->number crash

Metadata

Description

Reported by tilpner on IRC:

 $ csi
 
 CHICKEN
 (c) 2008-2015, The CHICKEN Team
 (c) 2000-2007, Felix L. Winkelmann
 Version 4.10.0 (rev b259631)
 linux-unix-gnu-x86-64 [ 64bit manyargs dload ptables ]
 compiled 2015-08-04 on yves.more-magic.net (Linux)
 
 #;1> (string->number "42" 60)
 
 Error: segmentation violation
 
         Call history:
 
         <syntax>          (string->number "42" 60)
         <eval>    (string->number "42" 60)      <--

Changes and comments

[2016-03-30 20:19:14 UTC] sjamaan wrote:

Oh my, this is nasty!

We first call C_strow() to attempt a conversion (and if it fails we roll our own). This is either strol() or stroll() depending on platform.

And what does the glibc manual state?

RETURN VALUE
      The  strtol() function returns the result of the conversion, unless the value would underflow or over‐
      flow.  If an underflow occurs, strtol() returns LONG_MIN.  If an  overflow  occurs,  strtol()  returns
      LONG_MAX.   In  both  cases,  errno  is  set  to ERANGE.  Precisely the same holds for strtoll() (with
      LLONG_MIN and LLONG_MAX instead of LONG_MIN and LONG_MAX).

ERRORS
      EINVAL (not in C99) The given base contains an unsupported value.

      ERANGE The resulting value was out of range.

      The implementation may also set errno to EINVAL in case no conversion was performed (no  digits  seen,
      and 0 returned).

So this breaks the spec (because ERANGE isn't supported by C99), and we only check for errno EINVAL. If errno has any other value, we dereference eptr, the end pointer which will have an unspecified value if it returned an error!

I'm not sure what the correct fix would be here. I suppose we should just ignore errno, or bail out if we see an unexpected one with (n == C_LONG_MAX || n == C_LONG_MIN). But what if the return value just happens to actually be that value?

Also, if we're using stroll, we should actually be checking for C_LLONG_MAX or C_LLONG_MIN instead of C_LONG_MAX/C_LONG_MIN.

In CHICKEN 5, this problem should be gone because this function has been completely replaced by a full numeric tower-aware version.

[2016-03-31 21:26:00 UTC] sjamaan wrote:

OK, my analysis of yesterday was a bit too hasty: C_LONG_MAX is also conditionally declared to be either LONG_MAX or LONG_LONG_MAX. This is all correct.

The handling of error situations seems to be the problem: the C_LONG_MAX/C_LONG_MIN values are returned iff the result is an overflow or underflow. However, the (draft) spec also says "If no conversion could be performed, zero is returned", and this is what is happening here. eptr is never even touched (though the spec doesn't say if this may happen).

[2016-03-31 21:30:06 UTC] sjamaan wrote:

Because the spec says nothing about what happens if the base is outside the supported 2..36 range, we should avoid passing in such values in the first place.

[2016-04-03 08:03:14 UTC] evhan changed status from new to closed

[2016-04-03 08:03:14 UTC] evhan set resolution to fixed

[2016-04-03 08:03:14 UTC] evhan wrote:

Fixed by 76a84c3 and 2d5ff60.