string->number crash

[Mario Domenech Goulart]

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)      <--

[sjamaan]

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.

[sjamaan]

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).

[sjamaan]

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.

[evhan]

Fixed by 76a84c3 and 2d5ff60.