Closed Bug 938615 Opened 7 years ago Closed 7 years ago

avoid GC hazard in ScriptSource::substring

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix

People

(Reporter: luke, Assigned: luke)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main28+])

Attachments

(1 file, 1 obsolete file)

Actually, I lied in bug 938390 comment 7; it looks like ScriptSource::substring doesn't AutoSupressGC and so a GC at js_NewStringCopyN<CanGC> could wipe out 'chars'.  I'll do what I was proposed in that bug.
Group: core-security
Attached patch fix-source-data-bug (obsolete) — Splinter Review
Attachment #832347 - Flags: review?(benjamin)
Comment on attachment 832347 [details] [diff] [review]
fix-source-data-bug

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

Why do SourceDataCache methods initialize the guard object rather than the constructor of AutoSupressPurge? To avoid pinning the cache if its not needed?
(In reply to :Benjamin Peterson from comment #2)
Oh, it's arbitrary, I suppose.  I definitely like having the methods take the AutoSuppressPurge argument b/c it forces you to think about it.  I suppose an alternative is to have the constructor of AutoSuppressPurge take a JSRuntime* and to the inc there and then just have the SourceDataCache methods assert it's the right one.  Do you think that would be better?
Oh, I definitely like that you have to pass the guard to chars(). I just thought it would be more deterministic if you also pinned the cache on construction of the guard. It would also prevent the guard from being initialized twice if you for some reason called put() or lookup() with the same guard.
Yes, that does sound nicer.
Attachment #832347 - Attachment is obsolete: true
Attachment #832347 - Flags: review?(benjamin)
Attachment #832458 - Flags: review?(benjamin)
Comment on attachment 832458 [details] [diff] [review]
fix-source-data-bug

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

Thanks for tidying that up.

::: js/src/jsscript.cpp
@@ +3184,5 @@
>      if (!scriptChars)
>          return false;
>  
> +    SourceDataCache::AutoSuppressPurge asp2(cx);
> +    const jschar *lazyChars = lazy->source()->chars(cx, asp2);

Presumably you can reuse |asp1| here?

::: js/src/jsscript.h
@@ +315,5 @@
> +    {
> +        SourceDataCache &cache_;
> +        mozilla::DebugOnly<size_t> oldValue_;
> +      public:
> +        AutoSuppressPurge(JSContext *cx);

explicit
Attachment #832458 - Flags: review?(benjamin) → review+
(In reply to :Benjamin Peterson from comment #6)
> Presumably you can reuse |asp1| here?

Oops, yes, now we can.
How far back does this problem go?
Looks like it's on release, at least.  For context, I think it would be pretty difficult to hit this bug.  I already checked crash-stats and nothing in JSScript::substring.
landed on https://hg.mozilla.org/mozilla-central/rev/63e0b6e8f84c
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-b2g18: --- → ?
status-b2g-v1.2: --- → ?
(In reply to Luke Wagner [:luke] from comment #10)
> Looks like it's on release, at least.  For context, I think it would be
> pretty difficult to hit this bug.  I already checked crash-stats and nothing
> in JSScript::substring.

One of the things we're trying to figure out is if it affects ESR24.

Is there a reason we didn't nominate this for Firefox 27 as a sec-high bug?
Flags: needinfo?(luke)
Since this was theoretical, not discovered, and likely never to happen in practice, I thought it was least risky to let it ride the trains.
Flags: needinfo?(luke)
Do we need this on ESR24 since it is a sec-high? It isn't clear if it affects ESR24 to me.
Flags: needinfo?(luke)
I assume ESR24 is affected.  The patch has some non-trivial rebase conflicts when attempting to apply it to mozilla-esr24 so I'd be really hesitant to attempt to backport this patch lest a separate crash/leak be created.
Flags: needinfo?(luke)
Ok. Thanks. Dan, what do you think about this for ESR24? Should we avoid it because of potential issues.
Flags: needinfo?(dveditz)
Let's wontfix for esr24, not worth the risk. I'm not sure about b2g1.2 (gecko26). Doesn't look like any partners are taking that version anyway so maybe just mark it affected for now and we'll worry about it later if necessary.
This would also require some rebasing to land on b2g26. Luke, you OK calling this wontfix there?
Flags: needinfo?(luke)
Yes, this bug is just theoretical.
Flags: needinfo?(luke)
Whiteboard: [adv-main28+]
Depends on: 986864
Group: core-security
You need to log in before you can comment on or make changes to this bug.