Closed Bug 1394530 Opened 7 years ago Closed 7 years ago

Assertion failure: this->is<T>(), at js/src/jsobj.h:575 with Promise

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 57+ fixed
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + verified
firefox58 + verified

People

(Reporter: decoder, Assigned: arai)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main57+][adv-esr52.5+])

Attachments

(2 files, 3 obsolete files)

Stop using optimized path for non PromiseObject. r=till
3.68 KB, patch
arai
: review+
Sylvestre
: approval-mozilla-beta+
Sylvestre
: approval-mozilla-esr52+
abillings
: sec-approval+
Details | Diff | Splinter Review
(do not land this now) Part 2: Add test cases. r=till
1.40 KB, patch
arai
: review+
Details | Diff | Splinter Review 
The following testcase crashes on mozilla-central revision d10c97627b51 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

evaluate(`
  $ERROR = function $ERROR(message) {};
  var thenable = {};
  var p1 = Promise.resolve(thenable);
  function resolveFunction() { }
  function Constructor(executor) {
    executor(resolveFunction, $ERROR);
  }
  Constructor.resolve = function(v) { return v; };
  var p2 = {};
  Promise.race.call(Constructor, [p1, p2]);
`);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000005c8a08 in JSObject::as<js::PromiseObject> (this=<optimized out>) at js/src/jsobj.h:575
#0  0x00000000005c8a08 in JSObject::as<js::PromiseObject> (this=<optimized out>) at js/src/jsobj.h:575
#1  RunResolutionFunction (cx=0x7ffff6924000, resolutionFun=resolutionFun@entry=..., result=result@entry=..., mode=mode@entry=ResolveMode, promiseObj=promiseObj@entry=...) at js/src/builtin/Promise.cpp:1871
#2  0x00000000005cf4d9 in PromiseReactionJob (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Promise.cpp:1212
#3  0x0000000000548c0b in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0x5cec80 <PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
[...]
#11 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8592
rax	0x0	0
rbx	0x7ffff6924000	140737330167808
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffd200	140737488343552
rsp	0x7fffffffd150	140737488343376
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffd350	140737488343888
r13	0x0	0
r14	0x7fffffffd310	140737488343824
r15	0x7fffffffd330	140737488343856
rip	0x5c8a08 <RunResolutionFunction(JSContext*, JS::HandleObject, JS::HandleValue, ResolutionMode, JS::HandleObject)+728>
=> 0x5c8a08 <RunResolutionFunction(JSContext*, JS::HandleObject, JS::HandleValue, ResolutionMode, JS::HandleObject)+728>:	movl   $0x0,0x0
   0x5c8a13 <RunResolutionFunction(JSContext*, JS::HandleObject, JS::HandleValue, ResolutionMode, JS::HandleObject)+739>:	ud2    


Marking s-s because this assertion has previously been associated with security-sensitive bugs.
maybe sec-high if we're confused about what kind of object we are operating on?
Keywords: sec-high
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/309ecb16acfe
parent:      320142:dc422956242b
user:        Till Schneidereit
date:        Mon Oct 10 16:57:29 2016 +0200
summary:     Bug 1313049 - Port Promise-related functions from self-hosted JS to C++. r=arai, f=bz

This iteration took 234.017 seconds to run.
Till, is bug 1313049 a likely regressor?
Blocks: 1313049
Flags: needinfo?(till)
looks like an optimization for promise object is applied to not-optimizable case?

we should keep one of the following condition (am I correct?):
  1. the result promise is PromiseObject
  2. resolve and reject are callable

if 1 is true, resolve and reject can be null, and that's the optimization.

but in the testcase resultPromise is Object and resolve and reject are null,
apparently because we use then-branch for the following code:

https://dxr.mozilla.org/mozilla-central/rev/04b6be50a2526c7a26a63715f441c47e1aa1f9be/js/src/builtin/Promise.cpp#3056-3062
>         if (C == PromiseCtor) {
>             addToDependent = false;
>         } else {
>             // 25.4.5.3., step 4.
>             if (!NewPromiseCapability(cx, C, &resultPromise, &resolveFun, &rejectFun, true))
>                 return false;
>         }

not yet figured out what the appropriate fix tho, we should check some more before applying optimization,
and also maybe add several more assertion about the class of promise object.
(just adding ` && resultPromise->is<PromiseObject>()` to the condition also works, but not sure if it's sufficient)
Track 56+/57+ as sec-high.
tracking-firefox56: ? → +
tracking-firefox57: ? → +
Flags: needinfo?(arai.unmht)
Priority: -- → P1
taking.
I'll post patch shortly after testing some.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
this fixes the testcase in comment #0.
this seems to hit the 2nd one:

evaluate(`
  $ERROR = function $ERROR(message) {};
  var thenable = {};
  var p1 = Promise.resolve(thenable);
  function resolveFunction() { }
  function Constructor(executor) {
    executor(resolveFunction, $ERROR);
  }
  var rs = [];
  Constructor.resolve = function(v) { return new Promise(r => rs.push(r)); };
  var p2 = {};
  Promise.all.call(Constructor, [p1, p2]);
  for (var r of rs) {
    r();
  }
`);
(updating obsolete comments)

here's reverse call graphs for RunResolutionFunction and NewReactionRecord.

RunResolutionFunction (promise: 5th: HandleObject promiseObj
 |                    (func:    2nd: HandleObject resolutionFun)
 |
 +- AbruptRejectPromise (promise: 3rd: HandleObject promiseObj)
 |   |                  (func:    HandleObject reject)
 |   +- Promise_static_all (from NewPromiseCapability) => OK
 |   |
 |   +- Promise_static_race (from NewPromiseCapability) => OK
 |   -
 |   +- AsyncFromSyncIteratorMethod (from CreatePromiseObjectWithoutResolutionFunctions) => OK
 |
 +- PromiseReactionJob (promise: reaction->promise())
 |                     (func:    reaction->getFixedSlot(ReactionRecordSlot_Resolve/Reject))
 |                     except isAsyncFunction or isAsyncGenerator
 |                     => see NewReactionRecord
 |
 +- PerformPromiseAll (promise: 4th: HandleObject promiseObj)
 |   |                (func:    5th: HandleObject resolve)
 |   +- Promise_static_all (from NewPromiseCapability) => OK
 |
 +- PromiseAllResolveElementFunction (promise: data->promiseObj())
 |   |                               (func:    data->resolveObj())
 |   |                               (from PromiseAllResolveElementFunctionSlot_Data)
 |   +- GetWaitForAllPromise (from NewPromiseCapability) => OK
 |   |
 |   |
 |   +- PerformPromiseAll (see above)
 |
 +- CommonStaticResolveRejectImpl (from NewPromiseCapability) => OK

NewReactionRecord (promise: 2nd: HandleObject resultPromise)
 |                (func:    5th: HandleObject resolve, 6th: HandleObject reject)
 |
 +- InternalAwait (promise: 3rd: HandleObject resultPromise)
 |   |            (func:    nullptr, nullptr)
 |   +- AsyncFunctionAwait (calls setIsAsyncFunction) => OK
 |   |
 |   +- AsyncGeneratorAwait (calls setIsAsyncGenerator) => OK
 |   |
 |   +- AsyncFromSyncIteratorMethod (from CreatePromiseObjectWithoutResolutionFunctions) => OK
 |   |
 |   +- AsyncGeneratorResumeNext (calls setIsAsyncGenerator) => OK
 |
 +- PerformPromiseThen (promise: 5th: HandleObject resultPromise)
 |   |                 (func:    6th: HandleObject resolve, 7th : HandleObject reject)
 |   +- GetWaitForAllPromise (from NewPromiseCapability) => OK
 |   |
 |   +- OriginalPromiseThen (from NewPromiseCapability only if createDependent, otherwise all nullptr) => OK
 |   |
 |   +- BlockOnPromise (promise: 3rd HandleObject blockedPromise_ or from NewPromiseCapability)
 |       |             (func:    nullptr or from NewPromiseCapability)
 |       |             => **** need more check (the bug in comment #0) ****
 |       |
 |       +- PerformPromiseAll (see above) => OK
 |       |
 |       +- PerformPromiseRace (from NewPromiseCapability) => OK
 |
 +- AddPromiseReaction (promise: 5th: HandleObject dependentPromise)
     |                 (func:    6th: HandleObject resolve, 7th : HandleObject reject)
     +- BlockOnPromise (promise: 3rd: HandleObject blockedPromise_)
                       (func:    nullptr)
                       => **** need more check (yet another bug?) ****


NewPromiseCapability already checks resolve/reject are callable.

There are 2 cases in BlockOnPromise that uses the result of NewPromiseCapability for promise and nullptr for resolve/reject, that is the bug here.

1st case:
https://dxr.mozilla.org/mozilla-central/rev/34e2566a71f160eb3c5c3d92626453852e818f18/js/src/builtin/Promise.cpp#3063-3085
>         RootedObject resultPromise(cx, blockedPromise_);
>         RootedObject resolveFun(cx);
>         RootedObject rejectFun(cx);
> ...
>         if (C == PromiseCtor) {
>             addToDependent = false;
>         } else {
>             // 25.4.5.3., step 4.
>             if (!NewPromiseCapability(cx, C, &resultPromise, &resolveFun, &rejectFun, true))
>                 return false;
>         }
> ...
>         if (!PerformPromiseThen(cx, promise, onFulfilled, onRejected, resultPromise,
>                                 resolveFun, rejectFun))

and 2nd case:
https://dxr.mozilla.org/mozilla-central/rev/34e2566a71f160eb3c5c3d92626453852e818f18/js/src/builtin/Promise.cpp#3138-3139
>     return AddPromiseReaction(cx, promise, UndefinedHandleValue, UndefinedHandleValue,
>                               blockedPromise, nullptr, nullptr, nullptr);


the 1st case can be fixed by just skipping mis-optimization.
this fixes the testcase in comment #0.
>         if (C == PromiseCtor && resultPromise->is<PromiseObject>()) {
>             addToDependent = false;
>         } else {
>             // 25.4.5.3., step 4.
>             if (!NewPromiseCapability(cx, C, &resultPromise, &resolveFun, &rejectFun, true))
>                 return false;
>         }
> ...
>         if (!addToDependent)
>             return true;

but in any case non-optimizable case flows into the 2nd case,
but it's just for extra debug info,
we're already skipping this if the depends-on object is not promise object.

I'm going to just ignore the case that blocked object is not promise object.
Flags: needinfo?(arai.unmht)
1. removed mis-optimization for non-promise case
  2. skipped generating debug info for non-promise case
  3. added assertion to NewReactionRecord that checks either:
     * resultPromise is PromiseObject
     * resolve and reject are callable
Attachment #8909092 - Attachment is obsolete: true
Attachment #8909095 - Flags: review?(till)
and test cases.
Attachment #8909096 - Flags: review?(till)
Comment on attachment 8909095 [details] [diff] [review]
Stop using optimized path for non PromiseObject.

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

Thank you!

::: js/src/builtin/Promise.cpp
@@ +2360,5 @@
>  NewReactionRecord(JSContext* cx, HandleObject resultPromise, HandleValue onFulfilled,
>                    HandleValue onRejected, HandleObject resolve, HandleObject reject,
>                    HandleObject incumbentGlobalObject)
>  {
> +    // Either of the following conditions should be met:

Nit: s/should/must/

@@ +2361,5 @@
>                    HandleValue onRejected, HandleObject resolve, HandleObject reject,
>                    HandleObject incumbentGlobalObject)
>  {
> +    // Either of the following conditions should be met:
> +    //   * resultPromise is PromiseObject

Nit: "is a"

@@ +2367,5 @@
> +    // except for Async Generator, there resultPromise can be nullptr.
> +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), resolve);
> +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), IsCallable(resolve));
> +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), reject);
> +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), IsCallable(reject));

I think it'd be ok to remove the first and third of these asserts: IsCallable will cover them just fine.
Attachment #8909095 - Flags: review?(till) → review+
Attachment #8909096 - Flags: review?(till) → review+
Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #16)
> @@ +2367,5 @@
> > +    // except for Async Generator, there resultPromise can be nullptr.
> > +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), resolve);
> > +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), IsCallable(resolve));
> > +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), reject);
> > +    MOZ_ASSERT_IF(resultPromise && !resultPromise->is<PromiseObject>(), IsCallable(reject));
> 
> I think it'd be ok to remove the first and third of these asserts:
> IsCallable will cover them just fine.

IsCallable doesn't check null-ness. and that will just crashes, instead of throwing assertion failure.
I'd like to keep the assertion there.  I'll file followup bug to add assertion to IsCallable, and move existing assertions there.

https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/js/src/jsapi.cpp#2883
> JS_PUBLIC_API(bool)
> JS::IsCallable(JSObject* obj)
> {
>     return obj->isCallable();
> }

        Attachment #8909095 -
        Attachment is obsolete: true

        Attachment #8909701 -
        Flags: review+

    
    

    
  


  

        Attachment #8909096 -
        Attachment is obsolete: true

        Attachment #8909702 -
        Flags: review+

    
    

    
  
Comment on attachment 8909701 [details] [diff] [review]
Stop using optimized path for non PromiseObject. r=till

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

code that crashes can be easily created.
not sure about doing more with it.

> Do comments in the patch, the check-in comment, or tests included in the patch
> paint a bulls-eye on the security problem?

yes, it says not-optimizable case hits the issue.

the testcase in part 2 clearly describes the problem, so it should be kept for a while.

> Which older supported branches are affected by this flaw?

52

> If not all supported branches, which bug introduced the flaw?

bug 1313049

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

haven't yet created, but should be easy.
same patch should be applicable, maybe with minor rebase for surronding code.

> How likely is this patch to cause regressions; how much testing does it need?

not likely.
it falls back to non-optimized safer path.

        Attachment #8909701 -
        Flags: sec-approval?

    
    

    
  
It's not impossible to take this for the 56 RC2 build, if you think that is best
But we are super late in the release cycle, so it may be better to keep this fix to 57. 
So my preference is to wontfix this for 56 if that's OK with Al and Dan.

    
    

    
  
sec-approval for trunk checkin on October 9, two weeks into the cycle. Do not check this in before that date, please.

We'll want a beta patch made and nominated for 57 and ESR52 at that time.

          status-firefox56:
          affected → wontfix

          status-firefox58:
          --- → affected

          tracking-firefox56:
          + → ---

          tracking-firefox58:
          --- → +

          tracking-firefox-esr52:
          ? → 57+
Whiteboard: [jsbugmon:update] → [jsbugmon:update[checkin on 10/9]

    
    

    
  
confirmed the patch works both on beta and esr52.

        Attachment #8909701 -
        Flags: sec-approval? → sec-approval+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/079ff8998fd50540948d3262a3a8b1f4d61b43e9
Flags: needinfo?(till)
Whiteboard: [jsbugmon:update[checkin on 10/9] → [jsbugmon:update
Whiteboard: [jsbugmon:update → [jsbugmon:update]

    
  
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

    
    

    
  
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2ed5e7fbf39e).

    
    

    
  
Comment on attachment 8909701 [details] [diff] [review]
Stop using optimized path for non PromiseObject. r=till

Approval Request Comment for mozilla-beta
> [Feature/Bug causing the regression]
bug 1313049

> [User impact if declined]
crash when opening crafted website, and possible more critical exploit, because of dereferencing arbitrary value

> [Is this code covered by automated tests?]
I have patch to add tests, but it's not yet landed.

> [Has the fix been verified in Nightly?]
not yet

> [Needs manual test from QE? If yes, steps to reproduce]
no

> [List of other uplifts needed for the feature/fix]
none

> [Is the change risky?]
less risky

> [Why is the change risky/not risky?]
it falls back to non-optimized safer path

> [String changes made/needed]
none

----

Approval Request Comment for mozilla-esr52
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
it's sec-high

> User impact if declined:
crash when opening crafted website, and possible more critical exploit, because of dereferencing arbitrary value

> Fix Landed on Version:
not yet (m-i for 58 now)

> Risk to taking this patch (and alternatives if risky):
less risky
it falls back to non-optimized safer path

> String or UUID changes made by this patch:
none

        Attachment #8909701 -
        Flags: approval-mozilla-esr52?

        Attachment #8909701 -
        Flags: approval-mozilla-beta?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/079ff8998fd5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox58:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

    
  
Status: RESOLVED → VERIFIED

          status-firefox58:
          fixed → verified

    
    

    
  
JSBugMon: This bug has been automatically verified fixed.

    
    

    
  
Comment on attachment 8909701 [details] [diff] [review]
Stop using optimized path for non PromiseObject. r=till

Fix a crash and a sec-high, taking it.
Should be in 57b8 and the next esr

        Attachment #8909701 -
        Flags: approval-mozilla-esr52?

        Attachment #8909701 -
        Flags: approval-mozilla-esr52+

        Attachment #8909701 -
        Flags: approval-mozilla-beta?

        Attachment #8909701 -
        Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main57+][adv-esr52.5+]

    
    

    
  
JSBugMon: This bug has been automatically verified fixed on Fx57
Depends on: 1483188
Group: core-security-release
Keywords: bugmon

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: