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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[André Bargull [:anba]]

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

Comment 1

[André Bargull [:anba]]

Attachment: bug1464475-setpending-handle.patch

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.

Comment 2

[André Bargull [:anba]]

Attachment: bug1464475-testcase.patch

Regression test case in a different patch, in case we need to land it separately from the actual patch.

Comment 3

[André Bargull [:anba]]

Attachment: bug1464475-handlify-interceptor.patch

It probably makes sense to change the error-interceptor API to use HandleValue, too.

Comment 4

[Daniel Veditz [:dveditz]]

sec-moderate assuming the error-interceptor is never going to be used for web content in a shipping release.

Comment 5

[Steve Fink [:sfink] [:s:]]

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.

Comment 6

[Steve Fink [:sfink] [:s:]]

For some reason, NIGHTLY_BUILD is *not* defined in automation. Even though the same code built locally defines it.

Comment 7

[Steve Fink [:sfink] [:s:]]

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?

Comment 8

[Steve Fink [:sfink] [:s:]]

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?

Comment 9

[Jeff Walden [:Waldo]]

(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.

Comment 10

[Steve Fink [:sfink] [:s:]]

(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.

Comment 11

[Steve Fink [:sfink] [:s:]]

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.

Comment 12

[Jon Coppeard (:jonco)]

(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.

Comment 13

[André Bargull [:anba]]

Attachment: bug1464475-setpending-handle.patch

Update patch to apply cleanly on inbound, carrying r+.

Comment 14

[André Bargull [:anba]]

Check-in needed only for part "bug1464475-setpending-handle.patch" for now, the other two parts will land later.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bfa09b6fb7dd7574833339c541a8e442103c790

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8bfa09b6fb7d

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8987528 [details] [diff] [review]
bug1464475-setpending-handle.patch

(patch checked-in in comment 15)

Comment 18

[Nicolas B. Pierron [:nbp]]

(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?

Comment 19

[Steve Fink [:sfink] [:s:]]

(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.

Comment 20

[Jason Orendorff [:jorendorff]]

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.

Comment 21

[André Bargull [:anba]]

Attachment: bug1464475-testcase.patch

Rebased "bug1464475-testcase.patch" to apply on cleanly on tip, carrying r+.

Comment 22

[André Bargull [:anba]]

Attachment: bug1464475-handlify-interceptor.patch

Rebased "bug1464475-handlify-interceptor.patch" to apply on cleanly on tip, carrying r+.

Comment 23

[André Bargull [:anba]]

Check-in needed for "bug1464475-testcase.patch" and "bug1464475-handlify-interceptor.patch".

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf56104baefe0acd5e4d38b8db3678f8e85b1c81

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca2360041774004bcb49562cb8ba008a9fd518e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5f35b230181e915aaaf1afe24a643703e66724

Does this still need to be leave-open? Please remove the keyword if not.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0ca236004177
https://hg.mozilla.org/mozilla-central/rev/9c5f35b23018

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on the last two patches when you get a chance. They graft cleanly as-landed.

Comment 27

[André Bargull [:anba]]

Given that this is a Nightly-only feature, does it still make sense to request uplift to Beta?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Ah, if it's all behind NIGHTLY_BUILD, then no need to uplift indeed. Thanks.