Bug 160322 – Undefined Behavior in JSValue cast from NaN

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jonathan Bedard]

JSValues can be constructed from doubles, and in some cases, are deliberately constructed with NaN values.

In circumstances where NaN is bound through the default JSValue constructor, however, an undefined conversion to int32_t occurs.  While the subsequent if statement should fail and construct the JSValue through the explicit double constructor, given that the deliberate use of NaN is fairly common, it seems that the jsNaN() function should immediately call the explicit double constructor both for efficiency and to prevent inadvertent suppressing of any other bugs which may be instantiating a JSValue with a NaN double.

[Jonathan Bedard]

Created attachment 284825 [details]
Patch

[Alexey Proskuryakov]

Do we still have an undefined behavior when the passed value just happens to be a NaN?

The compiler will not see it and thus won't do anything bad, presumably.

[Mark Lam]

(In reply to comment #2)
> Do we still have an undefined behavior when the passed value just happens to
> be a NaN?
> 
> The compiler will not see it and thus won't do anything bad, presumably.

jsNaN() calls JSValue(double), and JSValue(double) casts the double value to an int32_t, which is undefined according to http://stackoverflow.com/questions/3986795/what-is-the-result-of-casting-float-inf-inf-and-nan-to-integer-in-c.

[Mark Lam]

(In reply to comment #0)
> JSValues can be constructed from doubles, and in some cases, are
> deliberately constructed with NaN values.
> 
> In circumstances where NaN is bound through the default JSValue constructor,
> however, an undefined conversion to int32_t occurs.  While the subsequent if
> statement should fail and construct the JSValue through the explicit double
> constructor, given that the deliberate use of NaN is fairly common, it seems
> that the jsNaN() function should immediately call the explicit double
> constructor both for efficiency and to prevent inadvertent suppressing of
> any other bugs which may be instantiating a JSValue with a NaN double.

Jonathan,

1. Please put this info in the ChangeLog.  It would be better if you can include a url to the C++ spec that shows that the NaN/Inf to int cast is undefined behavior to confirm that this is the case.

2. Is your fix adequate?  What about NaNs and Infinities that naturally arise from arithmetic?  For example, see slow_path_mul and slow_path_div in CommonSlowPaths.cpp.

Perhaps the mode complete fix is to fix JSValue(double) instead?

[Keith Miller]

I think JSValue(double) is fine. While the cast from double -> int32 is undefined, we only use the int32 if, when cast back to a double it's the same as the original value. As long as the undefinedness of the double -> int32 cast doesn't do anything too crazy, like corrupt memory or other variables, which I think we can rely on, that code should be safe. Perhaps my trust in the saneness of double -> int32 is unfounded.

[Jonathan Bedard]

I will update the change log.

Before uploading a new patch, do we want to fix the undefined behavior in the case where NaN is passed into the JSValue constructor?

Casting a NaN to an int32_t is undefined behavior, however, if you take a look at JSCJSValueInlines.h, line 144, the cast int is immediately compared to the double which constructed it, meaning that the int is re-cast to a double.

If the original double is either NaN or infinity, this comparison will fail (the integer will not equal the value which constructed it) and the JSValue will default to it's explicit double constructor, which is safe behavior.

I only changed the explicit NaN creation because it had the advantage of both disambiguating the cast as well as eliminating a few unneeded instructions.  Changing the double constructor will result in more instructions and no undefined behavior.  That being said, even if one line of the code as it stands now has undefined behavior, the behavior of the constructor as a whole is defined.

[Mark Lam]

Jonathan, I suspect the compiler would have folded away the int casted check at the top of JSValue(double) when we pass it a constant PNaN.  Hence, your patch is not necessarily a perf improvement after all.  Did you actually see the compiler actually generate code for this check?  If so, let's take this patch.  If not, we can let it go.

Regarding the general case, I see Keith's point.  In order for it to be an issue, a compiler would have to do more work to convert the int back to a double that matches NaN / Inf rather than just letting the CPU do its thing.  Hence, it's probably not an issue in practice and we can ignore it.

[Jonathan Bedard]

It is possible that some compilers optimize out the double constructor.  Open source clang (I specify open source because this bug was discovered with open source clang, not the version shipped with the operating system) almost certainly does not, otherwise this behavior would never have been caught in the first place.  This is something I double check on.

I also agree with Kieth that in practice, the double constructor is not an issue, and can be ignored.

The question only question left, then, is whether the clarification of jsNaN() is worth it.  I do think that regardless, this is probably worth a comment in the JSValue(double) constructor.

[Jonathan Bedard]

Created attachment 284904 [details]
Patch

[Jonathan Bedard]

Investigation of the assembly along with a brief bit of profiling confirms that some versions of clang do not fold away the cast to int check, even though PNaN is a constant.  This may be because PNaN must first be passed through the bitwise_cast function.

In any case, I do think that the changes are worth making both for performance and ambiguity reasons.

[Mark Lam]

Comment on attachment 284904 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 284904 [details]
Patch

Clearing flags on attachment: 284904

Committed r203925: <http://trac.webkit.org/changeset/203925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.