Closed Bug 1464475 Opened Last year Closed 11 months ago

Rooting hazard in JSContext::setPendingException() because error interceptor can trigger GC

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- disabled
firefox62 --- disabled
firefox63 --- disabled
firefox64 --- disabled
firefox65 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Depends on 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage])

Attachments

(3 files, 3 obsolete files)

I got this cgc crash while working on bug 1433303: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcc635edadb5ab2748cb805a62ffcf3e2d6cfd14&selectedJob=180234434

And when I then tried to reproduce the error locally, the testErrorInterceptor.cpp JSAPI test crashed in JSObject::compartment() when called from JSContext::setPendingException(), because the "group_" of the object contained '0xbad0bad1'. I guess that means JSContext::setPendingException() needs to be changed to use HandleValue, because the error interceptor can trigger GC.


Note: The error interceptor is a Nightly-only feature!


Here's a similar stack trace from the regression test for this bug:
---
0x000000000047dec5 in JSObject::compartment (this=0x7ffff53b4040) at /home/andre/hg/mozilla-inbound/js/src/vm/JSObject.h:163
163         JSCompartment* compartment() const { return group_->compartment(); }


(gdb) bt
#0  0x000000000047dec5 in JSObject::compartment (this=0x7ffff53b4040) at /home/andre/hg/mozilla-inbound/js/src/vm/JSObject.h:163
#1  0x00000000008189e3 in JSContext::setPendingException (this=0x7ffff5f23000, v=...) at /home/andre/hg/mozilla-inbound/js/src/vm/JSContext-inl.h:456
#2  0x0000000000eed21d in js::ErrorToException (cx=0x7ffff5f23000, reportp=0x7fffffffb7c0, callback=0x10c66e0 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0)
    at /home/andre/hg/mozilla-inbound/js/src/jsexn.cpp:701
#3  0x0000000000ff4a43 in js::CompileError::throwError (this=0x7fffffffb7c0, cx=0x7ffff5f23000) at /home/andre/hg/mozilla-inbound/js/src/vm/ErrorReporting.cpp:55
#4  0x0000000000ff4e95 in js::ReportCompileError(JSContext*, js::ErrorMetadata&&, mozilla::UniquePtr<JSErrorNotes, JS::DeletePolicy<JSErrorNotes> >, unsigned int, unsigned int, __va_list_tag*) (
    cx=0x7ffff5f23000, metadata=<unknown type in /home/andre/hg/mozilla-inbound/js/src/build-debug-obj/dist/bin/jsapi-tests, CU 0x34b02e7, DIE 0x356524f>, notes=..., flags=0, errorNumber=71, 
    args=0x7fffffffb9f0) at /home/andre/hg/mozilla-inbound/js/src/vm/ErrorReporting.cpp:125
#5  0x000000000078ffad in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::errorAt (this=0x7fffffffccc8, offset=0, errorNumber=71)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:655
#6  0x00000000007a02ce in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::assignExpr (this=0x7fffffffccc8, inHandling=js::frontend::InAllowed, 
    yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, possibleError=0x0, invoked=js::frontend::ParserBase::PredictUninvoked)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:8383
#7  0x0000000000792bb1 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::expr (this=0x7fffffffccc8, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, 
    tripledotHandling=js::frontend::TripledotProhibited, possibleError=0x0, invoked=js::frontend::ParserBase::PredictUninvoked) at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:7943
#8  0x0000000000794085 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::expressionStatement (this=0x7fffffffccc8, yieldHandling=js::frontend::YieldIsName, 
    invoked=js::frontend::ParserBase::PredictUninvoked) at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:6145
#9  0x0000000000793a15 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem (this=0x7fffffffccc8, yieldHandling=js::frontend::YieldIsName, canHaveDirectives=true)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:7808
#10 0x0000000000790de6 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList (this=0x7fffffffccc8, yieldHandling=js::frontend::YieldIsName)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:4273
#11 0x00000000007cf848 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7fffffffccc8, globalsc=0x7fffffffc658)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:2285
#12 0x0000000001581524 in BytecodeCompiler::compileScript (this=0x7fffffffc6f8, environment=..., sc=0x7fffffffc658) at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:336
#13 0x0000000001581db6 in BytecodeCompiler::compileGlobalScript (this=0x7fffffffc6f8, scopeKind=js::ScopeKind::Global) at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:381
#14 0x0000000001582e45 in js::frontend::CompileGlobalScript (cx=0x7ffff5f23000, alloc=..., scopeKind=js::ScopeKind::Global, options=..., srcBuf=..., sourceObjectOut=0x0)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:617
#15 0x0000000000edc1f7 in Evaluate (cx=0x7ffff5f23000, scopeKind=js::ScopeKind::Global, env=..., optionsArg=..., srcBuf=..., rval=...) at /home/andre/hg/mozilla-inbound/js/src/jsapi.cpp:4848
#16 0x0000000000edc00f in JS::Evaluate (cx=0x7ffff5f23000, options=..., bytes=0x19d81d9 "0 = 0;", length=6, rval=...) at /home/andre/hg/mozilla-inbound/js/src/jsapi.cpp:4893
#17 0x000000000062f25b in JSAPITest::execDontReport (this=0x2d3ea20 <cls_testErrorInterceptorGC_instance>, bytes=0x19d81d9 "0 = 0;", 
    filename=0x19d81e0 "/home/andre/hg/mozilla-inbound/js/src/jsapi-tests/testErrorInterceptorGC.cpp", lineno=24) at /home/andre/hg/mozilla-inbound/js/src/jsapi-tests/tests.cpp:65
#18 0x00000000004b7971 in cls_testErrorInterceptorGC::run (this=0x2d3ea20 <cls_testErrorInterceptorGC_instance>, global=...)
    at /home/andre/hg/mozilla-inbound/js/src/jsapi-tests/testErrorInterceptorGC.cpp:24
#19 0x000000000062f7cb in main (argc=2, argv=0x7fffffffd898) at /home/andre/hg/mozilla-inbound/js/src/jsapi-tests/tests.cpp:132



(gdb) p group_
$1 = {<js::WriteBarrieredBase<js::ObjectGroup*>> = {<js::BarrieredBase<js::ObjectGroup*>> = {
      value = 0xbad0bad1f53aa490}, <js::WrappedPtrOperations<js::ObjectGroup*, js::WriteBarrieredBase<js::ObjectGroup*> >> = {<No data fields>}, <No data fields>}, <No data fields>}
---
Curious, I thought this is the kind of issue the rooting hazard analysis normally catches? Assuming NIGHTLY_BUILD is set for the hazard build, so the error-interceptor feature is actually compiled into the binary.
Attachment #8980691 - Flags: review?(sphink)
Attached patch bug1464475-testcase.patch (obsolete) — Splinter Review
Regression test case in a different patch, in case we need to land it separately from the actual patch.
Attachment #8980692 - Flags: review?(sphink)
It probably makes sense to change the error-interceptor API to use HandleValue, too.
Attachment #8980693 - Flags: review?(sphink)
Group: core-security → javascript-core-security
Priority: -- → P1
sec-moderate assuming the error-interceptor is never going to be used for web content in a shipping release.
Keywords: sec-moderate
Oops, sorry, this kinda slipped by my radar. Looking now. You are correct, this is *exactly* what the hazard analysis is supposed to catch. So there's something fishy going on.
For some reason, NIGHTLY_BUILD is *not* defined in automation. Even though the same code built locally defines it.
Comment on attachment 8980691 [details] [diff] [review]
bug1464475-setpending-handle.patch

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

I'm surprised this is all you had to change. Assuming it is -- is the hazard analysis ok with this now?
Attachment #8980691 - Flags: review?(sphink) → review+
Comment on attachment 8980692 [details] [diff] [review]
bug1464475-testcase.patch

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

::: js/src/jsapi-tests/testErrorInterceptorGC.cpp
@@ +4,5 @@
> +
> +namespace {
> +
> +// An interceptor that triggers GC:
> +struct ErroInterceptorWithGC : JSErrorInterceptor {

*Erro«r»InterceptorWithGC

Is the private inheritance intentional?
Attachment #8980692 - Flags: review?(sphink) → review+
Attachment #8980693 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #8)
> > +// An interceptor that triggers GC:
> > +struct ErroInterceptorWithGC : JSErrorInterceptor {
> 
> *Erro«r»InterceptorWithGC
> 
> Is the private inheritance intentional?

That isn't private.  If you use the 'struct' tag and inherit without specifying access, you get public inheritance.  If you use the 'class' tag and inherit without specifying access, you get private inheritance.

Whether reliance on this arguable quirk is desirable, and versus just being explicit about it, I dunno.  But that there above is public inheritance.
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #6)
> For some reason, NIGHTLY_BUILD is *not* defined in automation. Even though
> the same code built locally defines it.

Sorry, operator error! I happened to have an esr52 tree checked out when I pushed to try, and it didn't have NIGHTLY_BUILD. But a current tree does. So I'm back to trying to understand why the analysis is being stupid.
Ugh. This is an old problem that we really ought to come up with some solution to.

The problem is that setPendingException took a const Value&. That's basically a pointer to Value, and the Value could hold a pointer to a GCThing. It's basically JSObject**, and the analysis doesn't treat those as errors.

It does report them in a separate file when you take the reference of the Value in the first place, because it's dangerous for the very reason here.

Except that it doesn't seem to be seeing this one?! Hm, the CFG is

  Call(97,98, __temp_30 := ObjectValue(__temp_31**))
  Call(98,99, cx*.setPendingException(__temp_30))

Either that call-assignment of __temp_30 is hiding a reference, or passing it to setPendingException is. The messily verbose version of that argument passing is:

    {
     "Kind": "Var",
     "Variable": {
      "BlockId": { ... },
      "Kind": "Temp",
      "Name": [
        "__temp_30",
        "__temp_30"
       ]
      }
     }

I guess there's no type there. Looking it up:

  {
  "Variable": {
   "BlockId": { ... },
   "Kind": "Temp",
   "Name": [
     "__temp_30",
     "__temp_30"
    ]
   },
  "Type": {
   "Kind": "CSU",
   "Name": "JS::Value"
   }
  },

Wtf? It's just a JS::Value. The compiler is hiding the part where it takes an address. And it's not just because it's a temporary; I checked another example where it's passing in a Rooted<Value>, and it ends up dereferencing down to the Value during that call too!

In the past, I've fixed things like this by passing Value instead of const Value&, and in fact I thought that was the rule we'd been using. But the current source code makes that into a total lie. Which is scary, given that even the fallback mechanism (that we rarely look at) is missing it as well!

jonco: er, do you think we should go through the tree and convert all const Value& parameters to Value? Sadly, it would be a pretty big change. And the hazard analysis really ought to be doing better here, but even if it were, we wouldn't actually catch any of these unless we start paying attentions to refs.txt.
Flags: needinfo?(jcoppeard)
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #11)
I don't exactly understand what's happening here.  The CFG is incorrect?  Can you grab me later to talk about this on vidyo (or whatever)?

I think we use |const Value&| as this is better on 32 bit platforms.  Can we make the analysis treat |const Foo&| as Foo?  That would make this kind of unsafe reference show up as hazards.  But that won't work if the data we get from the compiler is wrong.
Flags: needinfo?(jcoppeard)
Update patch to apply cleanly on inbound, carrying r+.
Attachment #8980691 - Attachment is obsolete: true
Attachment #8987528 - Flags: review+
Check-in needed only for part "bug1464475-setpending-handle.patch" for now, the other two parts will land later.
Comment on attachment 8987528 [details] [diff] [review]
bug1464475-setpending-handle.patch

(patch checked-in in comment 15)
Attachment #8987528 - Flags: checkin+
(In reply to André Bargull [:anba] from comment #14)
> Check-in needed only for part "bug1464475-setpending-handle.patch" for now,
> the other two parts will land later.

André, What are we waiting for landing the other patches?

(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> Ugh. This is an old problem that we really ought to come up with some
> solution to.

Steve, is there another bug that is related? Should it block this issue?
Flags: needinfo?(sphink)
Flags: needinfo?(andrebargull)
(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> (In reply to Steve Fink [:sfink] [:s:] from comment #11)
> > Ugh. This is an old problem that we really ought to come up with some
> > solution to.
> 
> Steve, is there another bug that is related? Should it block this issue?

Filed bug 1497593, thanks.
Flags: needinfo?(sphink)
Depends on: 1497593
Steve, would you please land the remaining two patches? They are not security-sensitive, as the GC hazard was fixed by the patch that landed in comment 15.
Flags: needinfo?(andrebargull) → needinfo?(sphink)
Rebased "bug1464475-testcase.patch" to apply on cleanly on tip, carrying r+.
Attachment #8980692 - Attachment is obsolete: true
Attachment #9020422 - Flags: review+
Rebased "bug1464475-handlify-interceptor.patch" to apply on cleanly on tip, carrying r+.
Attachment #8980693 - Attachment is obsolete: true
Attachment #9020423 - Flags: review+
Check-in needed for "bug1464475-testcase.patch" and "bug1464475-handlify-interceptor.patch".

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf56104baefe0acd5e4d38b8db3678f8e85b1c81
Keywords: checkin-needed
Flags: needinfo?(sphink)
Flags: needinfo?(andrebargull)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/0ca236004177
https://hg.mozilla.org/mozilla-central/rev/9c5f35b23018
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta approval on the last two patches when you get a chance. They graft cleanly as-landed.
Flags: needinfo?(andrebargull)
Given that this is a Nightly-only feature, does it still make sense to request uplift to Beta?
Flags: needinfo?(ryanvm)
Ah, if it's all behind NIGHTLY_BUILD, then no need to uplift indeed. Thanks.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.