938615 - avoid GC hazard in ScriptSource::substring

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Attachment: fix-source-data-bug

Comment 2

[:Benjamin Peterson]

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?

Comment 3

[Luke Wagner [:luke]]

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

Comment 4

[:Benjamin Peterson]

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.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-source-data-bug

Yes, that does sound nicer.

Comment 6

[:Benjamin Peterson]

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

Comment 7

[Luke Wagner [:luke]]

(In reply to :Benjamin Peterson from comment #6)
> Presumably you can reuse |asp1| here?

Oops, yes, now we can.

Comment 8

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e0b6e8f84c

Comment 9

[Daniel Veditz [:dveditz]]

How far back does this problem go?

Comment 10

[Luke Wagner [:luke]]

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.

Comment 11

[Carsten Book [:Tomcat]]

landed on https://hg.mozilla.org/mozilla-central/rev/63e0b6e8f84c

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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

Comment 13

[Luke Wagner [: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.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Do we need this on ESR24 since it is a sec-high? It isn't clear if it affects ESR24 to me.

Comment 15

[Luke Wagner [: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.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Ok. Thanks. Dan, what do you think about this for ESR24? Should we avoid it because of potential issues.

Comment 17

[Daniel Veditz [: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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This would also require some rebasing to land on b2g26. Luke, you OK calling this wontfix there?

Comment 19

[Luke Wagner [:luke]]

Yes, this bug is just theoretical.