As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 937083 - Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:223 with OOM in js::CallJSNative
: Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:223 with...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla28
Assigned To: Christian Holler (:decoder)
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-11-11 04:14 PST by Christian Holler (:decoder)
Modified: 2014-02-12 04:57 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[crash-signature] Machine-readable crash signature (418 bytes, text/plain)
2013-11-11 04:18 PST, Christian Holler (:decoder)
no flags Details
js-CharsToNumber-oom.patch (2.65 KB, patch)
2013-12-06 09:17 PST, Christian Holler (:decoder)
shu: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-11-11 04:14:42 PST
The following testcase asserts on mozilla-central revision bc8c1eb0f2ba (threadsafe build, run with --fuzzing-safe):


oomAfterAllocations(0)('xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
Comment 1 User image Christian Holler (:decoder) 2013-11-11 04:18:00 PST
Created attachment 830134 [details]
[crash-signature] Machine-readable crash signature
Comment 2 User image Christian Holler (:decoder) 2013-12-06 07:44:25 PST
I investigated this a bit, and the trace for [@ setPendingException] before the assertion looks like this (on m-c rev ):

#0  JSContext::setPendingException (this=0x9495980, v=$jsval(-nan(0xfff85f7111de0))) at ../jscntxtinlines.h:362
#1  0x0845248e in js_ReportOutOfMemory (cxArg=0x9495980) at ../jscntxt.cpp:376
#2  0x086289c4 in JSRuntime::onOutOfMemory (this=0x947e628, p=0x0, nbytes=59, cx=0x9495980) at ../vm/Runtime.cpp:806
#3  0x080686ec in js::ThreadSafeContext::onOutOfMemory (this=0x9495980, p=0x0, nbytes=59) at ../../jscntxt.h:266
#4  0x0806c36e in js::MallocProvider<js::ThreadSafeContext>::malloc_ (this=0x9495980, bytes=59) at ../../vm/Runtime.h:608
#5  0x084e9640 in js_strtod (cx=0x9495980, s=0x9543698 u'x' <repeats 58 times>, send=0x954370c u"", ep=0xffffbc10, dp=0xffffbc00) at ../jsnum.cpp:1766
#6  0x084e8cd6 in CharsToNumber (cx=0x9495980, chars=0x9543698 u'x' <repeats 58 times>, length=58, result=0xffffbd58) at ../jsnum.cpp:1519
#7  0x084e8d89 in js::StringToNumber (cx=0x9495980, str='x' <repeats 58 times>, result=0xffffbd58) at ../jsnum.cpp:1531
#8  0x084e8e7d in js::NonObjectToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1542
#9  0x084e9046 in js::ToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1593
#10 0x084e9181 in js::ToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1613
#11 0x0855afae in ToUint32SlowImpl<JSContext, js::ToNumberSlow, JS::Handle<JS::Value> > (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../jsnum.cpp:1699
#12 0x084e9389 in js::ToUint32Slow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../jsnum.cpp:1709
#13 0x080651ee in JS::ToUint32 (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../../jsapi.h:1186
#14 0x080f5d07 in OOMAfterAllocations (cx=0x9495980, argc=1, vp=0x94fc620) at ../builtin/TestingFunctions.cpp:807
#15 0x085f4fe1 in js::CallJSNative (cx=0x9495980, native=0x80f5c60 <OOMAfterAllocations(JSContext*, unsigned int, jsval*)>, args=...) at ../jscntxtinlines.h:220


Now this frame seems interesting to me:

> #6  0x084e8cd6 in CharsToNumber (cx=0x9495980, chars=0x9543698 u'x' <repeats 58 times>, length=58, result=0xffffbd58) at ../jsnum.cpp:1519

The code there looks like this:

>    if (!js_strtod(cx, bp, end, &ep, &d) || SkipSpace(ep, end) != end)
>        *result = GenericNaN();
>    else
>        *result = d;

The call to [@ js_strtod] is failing due to OOM, but we don't propagate this failure properly it seems (CharsToNumber is void and *result is set to NaN). If I understand the code correctly, then we should somehow propagate the OOM.

Needinfo from shu because he touched the code. If you're not the right person, please needinfo someone who is :) I'm also happy to write a patch for this once someone can roughly tell me what the preferred solution is.
Comment 3 User image Shu-yu Guo [:shu] 2013-12-06 09:15:09 PST
Good catch, you're right that CharsToNumber doesn't propagate OOM correctly.

Feel free to write the patch and r? me.
Comment 4 User image Christian Holler (:decoder) 2013-12-06 09:17:56 PST
Created attachment 8343816 [details] [diff] [review]
js-CharsToNumber-oom.patch

Patch, passes jit-tests and confirmed to fix the bug.
Comment 5 User image Shu-yu Guo [:shu] 2013-12-06 09:22:13 PST
Comment on attachment 8343816 [details] [diff] [review]
js-CharsToNumber-oom.patch

Review of attachment 8343816 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: js/src/jsnum.cpp
@@ +1516,5 @@
>       */
>      const jschar *ep;
>      double d;
> +    if (!js_strtod(cx, bp, end, &ep, &d)) {
> +        *result = GenericNaN();

There shouldn't be a need to set *result to anything if we're returning false.

@@ +1517,5 @@
>      const jschar *ep;
>      double d;
> +    if (!js_strtod(cx, bp, end, &ep, &d)) {
> +        *result = GenericNaN();
> +         return false;

Nit: bad indentation?
Comment 6 User image Christian Holler (:decoder) 2013-12-06 09:43:16 PST
Adjusted as discussed on IRC, thanks for the quick review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa482552d1aa
Comment 7 User image Wes Kocher (:KWierso) 2013-12-06 18:30:31 PST
https://hg.mozilla.org/mozilla-central/rev/fa482552d1aa
Comment 8 User image Ioana (away) 2014-02-11 04:16:42 PST
I get "ReferenceError: oomAfterAllocations is not defined" with both the 11/12 nightly and 02/11 beta shells (Ubuntu 13.04 x86). No assertions or any other issues.
Comment 9 User image Jesse Ruderman 2014-02-12 04:37:29 PST
If you want to verify this, you'll need a debug build. (The "oomAfterAllocations" testing function is debug-only.)
Comment 10 User image Jesse Ruderman 2014-02-12 04:39:35 PST
Btw, oomAfterAllocations(N) testcases are notorious for being fragile, so I wouldn't recommend verifying them in general. N==0 here, so you can if you want.
Comment 11 User image Christian Holler (:decoder) 2014-02-12 04:54:20 PST
I agree with Jesse here. In general I wouldn't waste resources on verifying non-security OOM errors.
Comment 12 User image Ioana (away) 2014-02-12 04:57:22 PST
Thanks guys! I'll leave this kind of bugs alone then :D

Note You need to log in before you can comment on or make changes to this bug.