Bug 180297

Summary: JavaScriptCore: missing exception checks in Math functions that take more than one argument
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, ews-watchlist, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 180306, 180395    
Attachments:
Description Flags
patch
mark.lam: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
patch none

JF Bastien
Reported 2017-12-01 16:51:44 PST
Some functions don't check that evaluating the first argument worked, causing a missing exception check assertion.
Attachments
patch (6.15 KB, patch)
2017-12-01 16:58 PST, JF Bastien
mark.lam: review+
ews-watchlist: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.54 MB, application/zip)
2017-12-01 18:16 PST, EWS Watchlist
no flags
patch (6.62 KB, patch)
2017-12-01 21:12 PST, JF Bastien
no flags
JF Bastien
Comment 1 2017-12-01 16:57:03 PST
JF Bastien
Comment 2 2017-12-01 16:58:30 PST
Mark Lam
Comment 3 2017-12-01 17:02:32 PST
Comment on attachment 328188 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328188&action=review r=me > JSTests/stress/math-exceptions.js:5 > +try { Math.acos(foo, foo); } catch (e) { } I think you should catch the exception and verify that the right exception is thrown.
Saam Barati
Comment 4 2017-12-01 17:03:31 PST
Comment on attachment 328188 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328188&action=review > JSTests/stress/math-exceptions.js:38 > +try { Math.acos(foo, foo); } catch (e) { } > +try { Math.acosh(foo, foo); } catch (e) { } > +try { Math.asin(foo, foo); } catch (e) { } > +try { Math.asinh(foo, foo); } catch (e) { } > +try { Math.atan(foo, foo); } catch (e) { } > +try { Math.atanh(foo, foo); } catch (e) { } > +try { Math.atan2(foo, foo); } catch (e) { } > +try { Math.cbrt(foo, foo); } catch (e) { } > +try { Math.ceil(foo, foo); } catch (e) { } > +try { Math.clz32(foo, foo); } catch (e) { } > +try { Math.cos(foo, foo); } catch (e) { } > +try { Math.cosh(foo, foo); } catch (e) { } > +try { Math.exp(foo, foo); } catch (e) { } > +try { Math.expm1(foo, foo); } catch (e) { } > +try { Math.floor(foo, foo); } catch (e) { } > +try { Math.fround(foo, foo); } catch (e) { } > +try { Math.hypot(foo, foo); } catch (e) { } > +try { Math.imul(foo, foo); } catch (e) { } > +try { Math.log(foo, foo); } catch (e) { } > +try { Math.log1p(foo, foo); } catch (e) { } > +try { Math.log10(foo, foo); } catch (e) { } > +try { Math.log2(foo, foo); } catch (e) { } > +try { Math.max(foo, foo); } catch (e) { } > +try { Math.min(foo, foo); } catch (e) { } > +try { Math.pow(foo, foo); } catch (e) { } > +try { Math.random(foo, foo); } catch (e) { } > +try { Math.round(foo, foo); } catch (e) { } > +try { Math.sign(foo, foo); } catch (e) { } > +try { Math.sin(foo, foo); } catch (e) { } > +try { Math.sinh(foo, foo); } catch (e) { } > +try { Math.sqrt(foo, foo); } catch (e) { } > +try { Math.tan(foo, foo); } catch (e) { } > +try { Math.tanh(foo, foo); } catch (e) { } > +try { Math.trunc(foo, foo); } catch (e) { } Why not do some validation for all the things you expect to throw after first arg? Like you catch the exception you expect to catch
Saam Barati
Comment 5 2017-12-01 17:05:24 PST
(In reply to Mark Lam from comment #3) > Comment on attachment 328188 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328188&action=review > > r=me > > > JSTests/stress/math-exceptions.js:5 > > +try { Math.acos(foo, foo); } catch (e) { } > > I think you should catch the exception and verify that the right exception > is thrown. Oops. I repeated what Mark says. But I totally agree!
Mark Lam
Comment 6 2017-12-01 17:05:46 PST
Comment on attachment 328188 [details] patch r=me with test fix.
EWS Watchlist
Comment 7 2017-12-01 18:16:55 PST
Comment on attachment 328188 [details] patch Attachment 328188 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5461158 New failing tests: js/pic/cached-array-length-access.html
EWS Watchlist
Comment 8 2017-12-01 18:16:56 PST
Created attachment 328201 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
JF Bastien
Comment 9 2017-12-01 21:12:11 PST
Created attachment 328208 [details] patch Update tests to verify the exception being thrown is the one we expect.
WebKit Commit Bot
Comment 10 2017-12-01 21:44:08 PST
Comment on attachment 328208 [details] patch Clearing flags on attachment: 328208 Committed r225443: <https://trac.webkit.org/changeset/225443>
WebKit Commit Bot
Comment 11 2017-12-01 21:44:09 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12 2017-12-02 12:21:12 PST
Comment on attachment 328208 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328208&action=review > Source/JavaScriptCore/runtime/MathObject.cpp:157 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); This seems unneeded; I believe that if you omitted it out it would simply make the exception case slower and the non-exception case faster. > Source/JavaScriptCore/runtime/MathObject.cpp:269 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Ditto.
JF Bastien
Comment 13 2017-12-04 17:01:09 PST
Comment on attachment 328208 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328208&action=review >> Source/JavaScriptCore/runtime/MathObject.cpp:157 >> + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > This seems unneeded; I believe that if you omitted it out it would simply make the exception case slower and the non-exception case faster. I'll replace with scope.release() here and below (before the toNumber that doesn't need an exception check).
JF Bastien
Comment 14 2017-12-04 23:21:36 PST
(In reply to JF Bastien from comment #13) > Comment on attachment 328208 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328208&action=review > > >> Source/JavaScriptCore/runtime/MathObject.cpp:157 > >> + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > > > This seems unneeded; I believe that if you omitted it out it would simply make the exception case slower and the non-exception case faster. > > I'll replace with scope.release() here and below (before the toNumber that > doesn't need an exception check). https://bugs.webkit.org/show_bug.cgi?id=180395
Note You need to log in before you can comment on or make changes to this bug.