Closed
Bug 805294
Opened 12 years ago
Closed 12 years ago
RegExpCompartment::sweep should mark the atom if the entry isn't removed
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: luke, Assigned: luke)
Details
(Keywords: sec-high, Whiteboard: [adv-main19+])
Attachments
(2 files, 1 obsolete file)
2.34 KB,
patch
|
Details | Diff | Splinter Review | |
5.88 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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.]
Assignee | ||
Comment 1•12 years ago
|
||
This is a prerequisite for the next patch.
Attachment #674920 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
This time, looks green on try.
Updated•12 years ago
|
Keywords: sec-critical
Assignee | ||
Comment 8•12 years ago
|
||
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.
Updated•12 years ago
|
Keywords: sec-critical → sec-high
Comment 9•12 years ago
|
||
It sounds like maybe this is a regression from IGC? I'm marking flags as such, but feel free to update as appropriate.
status-firefox-esr10:
--- → unaffected
status-firefox16:
--- → wontfix
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
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.
Assignee | ||
Comment 13•12 years ago
|
||
(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!
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b495a47e51d
Updated•12 years ago
|
Comment 15•12 years ago
|
||
(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?
Comment 16•12 years ago
|
||
(if so, please nominate for uplift)
Assignee | ||
Comment 17•12 years ago
|
||
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.
Updated•12 years ago
|
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9b495a47e51d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
status-firefox-esr17:
--- → affected
Updated•12 years ago
|
Updated•11 years ago
|
status-b2g18:
--- → wontfix
Updated•11 years ago
|
Whiteboard: [adv-main19+]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•