Avoid clearing ArenaHeader::chunk before finalizing all GC things so it can be used in asserts

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
The bug mentioned in bug 671702 comment 21 is a preexisting one. In particular, we clear ArenaHeader::compartment right after the arena becomes empty before the GC finishes finalizing all arenas. This means that during a compartment GC IsAboutToBeFinalized is going to return false for any thing from the arena due to a compartment mismatch.

For now I close the bug as potentially it may lead to accessing deleted memory.
(Assignee)

Comment 1

6 years ago
Created attachment 548019 [details] [diff] [review]
v1

The patch preserve ArenaHeader::compartment during the GC. To distinguish empty unallocated arenas during the conservative stack scan the patch sets ArenaHeader::thingKind for such arenas to FINALIZE_LIMIT.
Attachment #548019 - Flags: review?(anygregor)
In IsAboutToBeFinalized we have

    JSCompartment *thingCompartment = reinterpret_cast<const Cell *>(thing)->compartment();
    JSRuntime *rt = cx->runtime;
    JS_ASSERT(rt == thingCompartment->rt);

Shouldn't we see a thingCompartment == NULL for every GC (global and comp)? Are we just lucky that it never happens?
I'm a little confused here too. I thought we were supposed to be structuring GCs so that if some function checked IsAboutToBeFinalized(x), then we could guarantee that x definitely hasn't been swept yet. All the IsAboutToBeFinalized calls happen in weak marking, js_SweepAtomState, or JSCompartment::sweep, and these are all run before we sweep any arenas.

I guess we do expose IsAboutToBeFinalized through JSAPI. Is this where the problem is happening?
(In reply to comment #3)
> I'm a little confused here too. I thought we were supposed to be structuring
> GCs so that if some function checked IsAboutToBeFinalized(x), then we could
> guarantee that x definitely hasn't been swept yet. All the
> IsAboutToBeFinalized calls happen in weak marking, js_SweepAtomState, or
> JSCompartment::sweep, and these are all run before we sweep any arenas.
> 
> I guess we do expose IsAboutToBeFinalized through JSAPI. Is this where the
> problem is happening?

The JSGC_MARK_END callback should be fine.
I guess the JSGC_FINALIZE_END callback is the problem? Well just from the naming, IsAboutToBeFinalized shouldn't be called after/during JSGC_FINALIZE_END.
I just checked all the IsAboutToBeFinalized uses in mxr. They all look fine except for one in XPCWrappedNative::FlatJSObjectFinalized. That one is only called in debug builds, but there's nothing to guarantee that the object you're calling it on hasn't already been swept, as far as I can see. We could probably just take it out.
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> I just checked all the IsAboutToBeFinalized uses in mxr. They all look fine
> except for one in XPCWrappedNative::FlatJSObjectFinalized.

You are right, I should have checked xpconnect better than relining on the memory. So there is no bug except that one in debug-only xpc. I suppose we can require that IsAboutToBeFinalized can only be called before the finalization itself begins. But that is for another bugs.

> That one is only
> called in debug builds, but there's nothing to guarantee that the object
> you're calling it on hasn't already been swept, as far as I can see. We
> could probably just take it out.

mrbkap probably now the reason behind the assert...

Gregor, could you still review the patch as it helps to minimize those small-per-compartment GC chunk patches?
Group: core-security
Summary: ArenaHeader::chunk must not be cleared until the GC finishes → Avoid clearing ArenaHeader::chunk before finalizing all GC things so it can be used in asserts
(Assignee)

Updated

6 years ago
Blocks: 673795
(In reply to comment #6)
> 
> Gregor, could you still review the patch as it helps to minimize those
> small-per-compartment GC chunk patches?

I don't see a connection here. Can you explain why this helps?
And also why do you think this patch makes more sense?
(Assignee)

Comment 9

6 years ago
(In reply to comment #7)
> I don't see a connection here. Can you explain why this helps?

For the bug 671702 I need to move the compartment pointer from ArenaHeader:: to ChunkInfo::. As such it is necessary to use something to denote unallocated arena instead of null compartment pointer. But this is also useful on its own for this bug as it allows to keep the compartment pointer as is after marking the arena as unallocated. So this patch replaces the !compartment check with thingKind == FINALIZE_LIMIT minimizing the amount of changes in the bug 671702.
Comment on attachment 548019 [details] [diff] [review]
v1


>+#ifdef DEBUG
>+        memset(a, ArenaSize, JS_FREE_PATTERN);
>+#endif
>+        *prevp = &a->aheader;
>+        a->aheader.initAsNotAllocated();

Please unify the setting with a single function.
something like setNotAllocated()?

>+    /*
>+     * We do not clear aheader->compartment so IsAboutToBeFinalized can use
>+     * the field until finalization finishes.
>+     */
>+    aheader->thingKind = FINALIZE_LIMIT;
>     aheader->next = info.emptyArenaListHead;




>+
>+    /*
>+     * We do not clear aheader->compartment so IsAboutToBeFinalized can use
>+     * the field until finalization finishes.
>+     */
>+    aheader->thingKind = FINALIZE_LIMIT;

I don't think this comment is true now.

Another comment in AnreaHeader describing the new meaning of thingKind and compartment would be good.
Attachment #548019 - Flags: review?(anygregor) → review+
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/862f7431e744
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/862f7431e744
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.