160322 2016-07-28 15:50:19 -0700 Undefined Behavior in JSValue cast from NaN 2016-07-29 16:43:27 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Minor --- 1 jbedard webkit-unassigned commit-queue ggaren keith_miller mark.lam msaboff saam oldest_to_newest 1215421 0 jbedard 2016-07-28 15:50:19 -0700 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. 1215424 1 284825 jbedard 2016-07-28 15:55:05 -0700 Created attachment 284825 Patch 1215649 2 ap 2016-07-29 12:11:34 -0700 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. 1215664 3 mark.lam 2016-07-29 12:45:36 -0700 (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. 1215668 4 mark.lam 2016-07-29 12:52:49 -0700 (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? 1215689 5 keith_miller 2016-07-29 13:26:35 -0700 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. 1215690 6 jbedard 2016-07-29 13:40:02 -0700 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. 1215695 7 mark.lam 2016-07-29 13:51:35 -0700 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. 1215699 8 jbedard 2016-07-29 14:14:44 -0700 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. 1215710 9 284904 jbedard 2016-07-29 14:44:45 -0700 Created attachment 284904 Patch 1215726 10 jbedard 2016-07-29 15:51:20 -0700 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. 1215727 11 284904 mark.lam 2016-07-29 15:53:27 -0700 Comment on attachment 284904 Patch r=me 1215750 12 284904 commit-queue 2016-07-29 16:43:23 -0700 Comment on attachment 284904 Patch Clearing flags on attachment: 284904 Committed r203925: <http://trac.webkit.org/changeset/203925> 1215751 13 commit-queue 2016-07-29 16:43:27 -0700 All reviewed patches have been landed. Closing bug. 284825 2016-07-28 15:55:05 -0700 2016-07-29 14:44:42 -0700 Patch bug-160322-20160728155352.patch text/plain 1147 jbedard SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjAzODQ3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDE2LTA3LTI4ICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNvbT4KKworICAg ICAgICBVbmRlZmluZWQgQmVoYXZpb3IgaW4gSlNWYWx1ZSBjYXN0IGZyb20gTmFOCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjAzMjIKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvSlNDSlNW YWx1ZUlubGluZXMuaDoKKyAgICAgICAgKEpTQzo6anNOYU4pOiBFeHBsaWNpdCBkb3VibGUgY29u c3RydWN0aW9uIGZvciBOYU4gSlNWYWx1ZXMgdG8gYXZvaWQgdW5kZWZpbmVkIGJlaGF2aW9yLgor CiAyMDE2LTA3LTI4ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBT dHJpbmdWaWV3IHNob3VsZCBoYXZlIGFuIGV4cGxpY2l0IG1faXM4Qml0IGZpZWxkLgpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDSlNWYWx1ZUlubGluZXMuaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0NKU1ZhbHVlSW5saW5lcy5o CShyZXZpc2lvbiAyMDM4MzApCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0NK U1ZhbHVlSW5saW5lcy5oCSh3b3JraW5nIGNvcHkpCkBAIC03MCw3ICs3MCw3IEBAIGlubGluZSBk b3VibGUgSlNWYWx1ZTo6YXNOdW1iZXIoKSBjb25zdAogCiBpbmxpbmUgSlNWYWx1ZSBqc05hTigp CiB7Ci0gICAgcmV0dXJuIEpTVmFsdWUoUE5hTik7CisgICAgcmV0dXJuIEpTVmFsdWUoSlNWYWx1 ZTo6RW5jb2RlQXNEb3VibGUsIFBOYU4pOwogfQogCiBpbmxpbmUgSlNWYWx1ZTo6SlNWYWx1ZShj aGFyIGkpCg== 284904 2016-07-29 14:44:45 -0700 2016-07-29 16:43:23 -0700 Patch bug-160322-20160729144331.patch text/plain 2242 jbedard SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjAzOTA1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBA CisyMDE2LTA3LTI4ICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNvbT4KKworICAg ICAgICBVbmRlZmluZWQgQmVoYXZpb3IgaW4gSlNWYWx1ZSBjYXN0IGZyb20gTmFOCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjAzMjIKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBKU1ZhbHVlcyBjYW4gYmUg Y29uc3RydWN0ZWQgZnJvbSBkb3VibGVzLCBhbmQgaW4gc29tZSBjYXNlcywgYXJlIGRlbGliZXJh dGVseSBjb25zdHJ1Y3RlZCB3aXRoIE5hTiB2YWx1ZXMuCisKKyAgICAgICAgSW4gY2lyY3Vtc3Rh bmNlcyB3aGVyZSBOYU4gaXMgYm91bmQgdGhyb3VnaCB0aGUgZGVmYXVsdCBKU1ZhbHVlIGNvbnN0 cnVjdG9yLCBob3dldmVyLCBhbiB1bmRlZmluZWQgY29udmVyc2lvbgorICAgICAgICB0byBpbnQz Ml90IG9jY3Vycy4gIFdoaWxlIHRoZSBzdWJzZXF1ZW50IGlmIHN0YXRlbWVudCBzaG91bGQgZmFp bCBhbmQgY29uc3RydWN0IHRoZSBKU1ZhbHVlIHRocm91Z2ggdGhlIGV4cGxpY2l0CisgICAgICAg IGRvdWJsZSBjb25zdHJ1Y3RvciwgZ2l2ZW4gdGhhdCB0aGUgZGVsaWJlcmF0ZSB1c2Ugb2YgTmFO IGlzIGZhaXJseSBjb21tb24sIGl0IHNlZW1zIHRoYXQgdGhlIGpzTmFOKCkgZnVuY3Rpb24KKyAg ICAgICAgc2hvdWxkIGltbWVkaWF0ZWx5IGNhbGwgdGhlIGV4cGxpY2l0IGRvdWJsZSBjb25zdHJ1 Y3RvciBib3RoIGZvciBlZmZpY2llbmN5IGFuZCB0byBwcmV2ZW50IGluYWR2ZXJ0ZW50CisgICAg ICAgIHN1cHByZXNzaW5nIG9mIGFueSBvdGhlciBidWdzIHdoaWNoIG1heSBiZSBpbnN0YW50aWF0 aW5nIGEgSlNWYWx1ZSB3aXRoIGEgTmFOIGRvdWJsZS4KKworICAgICAgICAqIHJ1bnRpbWUvSlND SlNWYWx1ZUlubGluZXMuaDoKKyAgICAgICAgKEpTQzo6anNOYU4pOiBFeHBsaWNpdCBkb3VibGUg Y29uc3RydWN0aW9uIGZvciBOYU4gSlNWYWx1ZXMgdG8gYXZvaWQgdW5kZWZpbmVkIGJlaGF2aW9y LgorCiAyMDE2LTA3LTI5ICBZdXN1a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21haWwuY29tPgog CiAgICAgICAgIFVucmV2aWV3ZWQsIEJ5VmFsSW5mbyBpcyBvbmx5IHVzZWQgaW4gSklUIGVuYWJs ZWQgZW52aXJvbm1lbnRzCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0NK U1ZhbHVlSW5saW5lcy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50 aW1lL0pTQ0pTVmFsdWVJbmxpbmVzLmgJKHJldmlzaW9uIDIwMzkwNSkKKysrIFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0pTQ0pTVmFsdWVJbmxpbmVzLmgJKHdvcmtpbmcgY29weSkKQEAg LTcwLDcgKzcwLDcgQEAgaW5saW5lIGRvdWJsZSBKU1ZhbHVlOjphc051bWJlcigpIGNvbnN0CiAK IGlubGluZSBKU1ZhbHVlIGpzTmFOKCkKIHsKLSAgICByZXR1cm4gSlNWYWx1ZShQTmFOKTsKKyAg ICByZXR1cm4gSlNWYWx1ZShKU1ZhbHVlOjpFbmNvZGVBc0RvdWJsZSwgUE5hTik7CiB9CiAKIGlu bGluZSBKU1ZhbHVlOjpKU1ZhbHVlKGNoYXIgaSkKQEAgLTE0MCw2ICsxNDAsNyBAQCBpbmxpbmUg SlNWYWx1ZTo6SlNWYWx1ZSh1bnNpZ25lZCBsb25nIGxvCiAKIGlubGluZSBKU1ZhbHVlOjpKU1Zh bHVlKGRvdWJsZSBkKQogeworICAgIC8vIE5vdGU6IHdoaWxlIHRoaXMgYmVoYXZpb3IgaXMgdW5k ZWZpbmVkIGZvciBOYU4gYW5kIGluZiwgdGhlIHN1YnNlcXVlbnQgc3RhdGVtZW50IHdpbGwgY2F0 Y2ggdGhlc2UgY2FzZXMuCiAgICAgY29uc3QgaW50MzJfdCBhc0ludDMyID0gc3RhdGljX2Nhc3Q8 aW50MzJfdD4oZCk7CiAgICAgaWYgKGFzSW50MzIgIT0gZCB8fCAoIWFzSW50MzIgJiYgc3RkOjpz aWduYml0KGQpKSkgeyAvLyB0cnVlIGZvciAtMC4wCiAgICAgICAgICp0aGlzID0gSlNWYWx1ZShF bmNvZGVBc0RvdWJsZSwgZCk7Cg==