Bug 180297 – JavaScriptCore: missing exception checks in Math functions that take more than one argument

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

[JF Bastien]

Some functions don't check that evaluating the first argument worked, causing a missing exception check assertion.

[JF Bastien]

<rdar://problem/35745556>

[JF Bastien]

Created attachment 328188 [details]
patch

[Mark Lam]

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

(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 on attachment 328188 [details]
patch

r=me with test fix.

[EWS Watchlist]

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]

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]

Created attachment 328208 [details]
patch

Update tests to verify the exception being thrown is the one we expect.

[WebKit Commit Bot]

Comment on attachment 328208 [details]
patch

Clearing flags on attachment: 328208

Committed r225443: <https://trac.webkit.org/changeset/225443>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

(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