1616909 - Hazard Introduced by SourceExtent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

It appears that as part of my SourceExtent Patch (Bug 1615728) I introduced a GC hazard

The issue is as follows:

1. In JSScript::CreateFromLazy we call JSScript::New(cx, fun, sourceObject, lazy->extent()); lazy->extent() returns a reference to the member extent_ of the correctly rooted LazyScript lazy.
2. Inside JSScript::New we call Allocate, then invoke the JSScript constructor on the returned memory, with the extent reference that was passed in.
3. When Allocate is called, lazy gets moved; and the reference now points to deallocated memory.

I'm going to mark this as a secure bug; I'm not entirely sure of the security implications here, and given it's only been on nightly for less than 24 hours it's not high sev, but it's an issue.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

On my local build I can reproduce this trivally by running central with the anyref-val-tracing test.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1616909 - Return a copy of SourceExtent, not a reference r?djvj

Comment 3

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

For the few times we return references or pointers to fields of GC things, the quick & dirty technique is to change the accessor signature to require an AutoRequireNoGC& parameter. It doesn't fully prevent problems, but at least makes the caller create (probably) an AutoCheckCannotGC on the stack and thus think about the live range and where GC is possible.

...but of course, you have to know about the potential for problems in order to know to do something like that, and I've got nothing for that.

(And your fix here is definitely better; if you can get away with copying, it's safer and easier to use.)

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

I opened Bug 1616926 as a potential road for us to get a simple static analysis that helps prevent cases like this.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Pushed https://hg.mozilla.org/integration/autoland/rev/73b4ee52907acc2ed6f6743f6894c78960cb744c

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/73b4ee52907a