Assertion failure: !JS_IsDeadWrapper(obj), at gecko-dev/js/src/gc/FinalizationObservers.cpp:409
Categories
(Core :: JavaScript: GC, defect, P2)
Tracking
()
People
(Reporter: anbu1024.me, Assigned: sfink)
References
(Blocks 1 open bug)
Details
(Keywords: reporter-external, sec-moderate, Whiteboard: [adv-main119+][adv-ESR115.4+])
Attachments
(2 files, 1 obsolete file)
|
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
dmeehan
:
approval-mozilla-esr115+
|
Details | Review |
|
245 bytes,
text/plain
|
Details |
Steps to reproduce:
SpiderMonkey:
commit: 09f7e00da9eaef55c80620c17e8575ddd4532451
Build:
/bin/sh gecko-dev/js/src/configure --enable-debug --disable-optimize --disable-shared-js --disable-tests
Test case:
for (let i = 0, j = 16; i < j; )
{
j--;
Uint32Array.newCompartment = true;
const foo = this.newGlobal(Uint32Array).WeakRef;
for (let a = 0, b = 16; a < b; b--)
{
const bar = new foo(this.transplantableObject().object);
bar.deref(bar);
}
}
this.nukeAllCCWs();
Actual results:
Exec:
Assertion failure: !JS_IsDeadWrapper(obj), at gecko-dev/js/src/gc/FinalizationObservers.cpp:409
...
JS::NonIncrementalGC(JSContext*, JS::GCOptions, JS::GCReason)[gecko-dev-build/debug_09f7e0/dist/bin/js +0x30e26ff]
...
Segmentation fault (core dumped)
Updated•2 years ago
|
Comment 1•2 years ago
|
||
Steve, would you be willing to take a look at this new bug?
Updated•2 years ago
|
| Assignee | ||
Comment 2•2 years ago
•
|
||
Roughly what is going on:
- make a WeakRef to a target
TransplantableDOMObjectin another Zone - enter it into that Zone's
FinalizationObservers'weakRefMap: target -> list of WeakRef CCWs - while
deref'ing the WeakRef, call the read barrier - in the read barrier, the target
isDOMClass()and the DOM wrapper is considered to be "released", and the WeakRef target is cleared. I don't really know what this is all about, but I reviewed it when it was added so I'll pretend like I do. - nuke all cross-compartment wrappers. Because the WeakRef no longer has a target, it doesn't get cleared out of the
weakRefMap - crash when a validity check notices that
weakRefMapcontains a dead wrapper
I'm kind of out of my depth here, but I'll attach a preliminary patch that at least fixes the test case.
| Assignee | ||
Comment 3•2 years ago
|
||
Updated•2 years ago
|
| Assignee | ||
Updated•2 years ago
|
Comment 4•2 years ago
|
||
What are the security implications of this? I'm trying to figure out a rating.
| Assignee | ||
Comment 5•2 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #4)
What are the security implications of this? I'm trying to figure out a rating.
Yeah, I just went to land this, and realized that I needed to come up with a rating.
I haven't thought through it very carefully, but this doesn't seem very worrisome to me. It is sort of like a use after free, because we could do some finalization work on an object that no longer needs finalization. But the object is a dead object proxy, so I don't think it would really do very much. In particular, I haven't traced through but I can't think of a way for it to access freed memory. There might also be a trivial leak due to the failure to remove an entry from the cross-compartment weakref table.
It's still complicated and weird enough that I would probably keep it sec-something. I find it hard to predict exactly what could happen.
Comment 6•2 years ago
|
||
I guess I'll mark it sec-moderate, though we're not entirely sure what the implications might be.
Comment 8•2 years ago
|
||
Comment 9•2 years ago
|
||
The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox119towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Comment 10•2 years ago
•
|
||
Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared
Beta/Release Uplift Approval Request
- User impact if declined: Crashes, maybe some weird behavior. Possibly security problems, though I haven't come up with a scenario.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Medium
- Why is the change risky/not risky? (and alternatives if risky): The risk is low to medium just because this is complicated code and I guess I wouldn't be horribly surprised if it revealed some other problem. But: it only kicks in for unusual circumstances, the current behavior is clearly wrong, and the updated behavior is removing things from consideration when they get weird, resulting in less overall weirdness. While I appreciate weirdness in the wider world, I prefer to avoid it in Firefox's behavior.
- String changes made/needed: none
- Is Android affected?: Yes
Comment 11•2 years ago
|
||
Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared
Approved for 119.0b6
Comment 12•2 years ago
|
||
| uplift | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 13•2 years ago
|
||
Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared
Approved for 115.4esr.
Comment 14•2 years ago
|
||
| uplift | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 15•1 years ago
|
||
Comment 16•1 years ago
|
||
Updated•1 years ago
|
Comment 17•1 year ago
|
||
Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter
Comment 18•11 months ago
|
||
Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons
Description
•