667739 - Converting doubles to integers by checking for !finite, then comparing widen(narrow(d)) == d to check, relies on undefined behavior

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Consider js_DoubleToECMAUint32, among others:

> uint32
> js_DoubleToECMAUint32(jsdouble d)
> {
>     int32 i;
>     JSBool neg;
>     jsdouble two32;
> 
>     if (!JSDOUBLE_IS_FINITE(d))
>         return 0;
> 
>     /*
>      * We check whether d fits int32, not uint32, as all but the ">>>" bit
>      * manipulation bytecode stores the result as int, not uint. When the
>      * result does not fit int Value, it will be stored as a negative double.
>      */
>     i = (int32) d;
>     if ((jsdouble) i == d)
>         return (int32)i;

This code assumes that converting a finite-valued double to an integer, then converting that integer back to double, is a proper way to convert an integer-valued double to the corresponding integer.  It's not.  Quoting ISO C++, 4.9 Floating-integral conversions:

> An rvalue of a floating point type can be converted to an rvalue of an
> integer type. The conversion truncates; that is, the fractional part is
> discarded. The behavior is undefined if the truncated value cannot be
> represented in the destination type.

If the double is a value which doesn't fit in the integral type after truncation -- say, |d = 2**44| in the above code -- "behavior is undefined".  So *anything* could happen, not just that |i| holds a bogus value.

If you make certain assumptions about what the undefined behavior can be -- that i becomes garbage, and nothing else happens -- this is "safe".  But that seems pretty dangerous, especially in the face of PGO behaviors and such.  We should fix this.

Comment 1

[Nicholas Nethercote [inactive]]

TraceRecorder::makeNumberInt32() does the same thing, AFAICT.

Comment 2

[Jeff Walden [:Waldo]]

ToInt32/ToUint32 and friends were rewritten in bug 798179 to examine the underlying bits, and therefore never perform one of these unsafe casts.  TraceRecorder is long gone.  There may be other places along these lines, but I don't know 'em offhand, so it seems dumb to leave this bug open without knowing.  If other places emerge, we should fix them in separate bugs.