Closed Bug 805294 Opened 7 years ago Closed 7 years ago

RegExpCompartment::sweep should mark the atom if the entry isn't removed

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox16 --- wontfix
firefox17 + wontfix
firefox18 + wontfix
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: luke, Assigned: luke)

Details

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

Attachments

(2 files, 1 obsolete file)

Normally, we clear out all entries of the regexp cache.  The exception is when a regexp is currently executing and then that regexp gets to stay in the cache.  In this case, we fail to mark the JSAtom that serves as the key, so it can become garbage.  IIUC, we don't use the key other than for equality matching, so this error only manifests itself as the cache returning the wrong regexp for the given source (from the same compartment, so not an information leak).

I tried to write a test-case, but the conservative GC thwarted all my attempts, since the window of opportunity is small.  [Credit to Ginn Chen for reproducing and tracking down this bug.]
This is a prerequisite for the next patch.
Attachment #674920 - Flags: review?(wmccloskey)
Attached patch mark the atom (obsolete) — Splinter Review
Ginn, can you verify that this patch (applied on top of the other patch) fixes the problem you are seeing?
Attachment #674921 - Flags: review?(wmccloskey)
Attachment #674921 - Flags: feedback?(ginn.chen)
Comment on attachment 674921 [details] [diff] [review]
mark the atom

The problem is gone with both patch applied.
Thanks!
Attachment #674921 - Flags: feedback?(ginn.chen) → feedback+
So this fails all the mochitests.  The cause is a RegExpObject with a dangling private pointer to a deleted RegExpShared.  I think the reason is that JSCompartment::purge() gets called in some cases where we don't end up tracing the compartment.  Bill: should I condition RegExpCompartment::purge on something so that it purges iff we trace the compartment?
Comment on attachment 674921 [details] [diff] [review]
mark the atom

This patch has a flagrant igc violation: we purge RegExpShareds in the first slice, but we may not null out pointers to RegExpShareds until some later slice, leaving us completely unsafe in the interim.
Attachment #674921 - Flags: review?(wmccloskey)
Attached patch fixSplinter Review
To address the previous problem, this patch keeps two tables: a map that holds the atom that is unconditionally cleared, and a pendingDeletes_ set that is responsible for deleting RegExpShareds.
Attachment #674921 - Attachment is obsolete: true
Attachment #675637 - Flags: review?(wmccloskey)
This time, looks green on try.
I'm pretty sure this is, at most, sec-high.  While there is a dangling pointer, it serves only to allow same-origin incorrect regexp execution.  I'm not aware of any way that the dangling-pointer is actually dereferenced.
It sounds like maybe this is a regression from IGC? I'm marking flags as such, but feel free to update as appropriate.
Comment on attachment 675637 [details] [diff] [review]
fix

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

Sorry for the delay. I forgot about this after the work week.

::: js/src/jscompartment.cpp
@@ -562,1 @@
>          regExps.sweep(rt);

I never understood this comment before, but now I think I do. A JIT chunk can hold on to a RegExpShared by keeping its activeUseCount incremented. We can't throw that RegExpShared away until we discard JIT code. So it's beneficial to discard JIT code before sweeping regexps. Otherwise we would have to wait until the next GC to throw the RegExpShared away. Could you maybe clarify the comment, rather than deleting it? I think it does have some value, since I often shuffle this code around without much thought.

::: js/src/vm/RegExpObject.h
@@ +261,5 @@
>      typedef HashMap<Key, RegExpShared *, Key, RuntimeAllocPolicy> Map;
>      Map map_;
>  
> +    typedef HashSet<RegExpShared *, DefaultHasher<RegExpShared*>, RuntimeAllocPolicy> PendingSet;
> +    PendingSet pendingDeletes_;

How about calling this inUse_ or something like that? And it would be nice to have a one-line comment above the definitions of map_ and inUse_ (or whatever).
Attachment #675637 - Flags: review?(wmccloskey) → review+
Comment on attachment 674920 [details] [diff] [review]
unmark before purge

If I'm not mistaken, we no longer need this patch.
Attachment #674920 - Flags: review?(wmccloskey)
Also, I just checked, and Ion does not do the crazy inlining of regexp creation that JM does. It never needs to touch RegExpShared at all.
(In reply to Bill McCloskey (:billm) from comment #10)
> I never understood this comment before, but now I think I do. A JIT chunk
> can hold on to a RegExpShared by keeping its activeUseCount incremented.

Ohhhh, I thought this was an abandoned relic of the purely-ref-counted RegExpShared days.  You're right, it's totally still relevant.  Thanks!
(In reply to Luke Wagner [:luke] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9b495a47e51d

Beta 5, going to build Tuesday, will be the final cutoff for non-exploited security fixes. Is this fix low risk enough to take in FF17?
(if so, please nominate for uplift)
The risk isn't zero and the reward is also pretty low.  I was not even able to generate an observably-wrong test-case on x64 due to conservative stack scanning (the report came from an existing test on a non-tier 1 platform).  Thus, I recommend against uplift.
https://hg.mozilla.org/mozilla-central/rev/9b495a47e51d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.