Closed Bug 1118469 Opened 5 years ago Closed 5 years ago

Detect hazards in UniquePtr storage

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

There was recently an undetected hazard that boiled down to:

  {
    UniquePtr<RelocatablePtr<JSObject*>> objPtr = heapList.pop();
    canGC();
  }

The problem here is that the heapList is normally tracing the object, but we remove it from the heap without a prebarrier firing. Then the GC finishes, finds that the object is dead, and collects it. Finally, ~UniquePtr fires, which calls ~RelocatablePtr, which accesses the dead object.

The analysis sees nothing wrong because the stack only has a JSObject**. There isn't even any reference being taken here (the JSObject** gets copied from the heapList).

But this *is* a hazard, because UniquePtr memory is not traced. The UniquePtr won't trace it because it's too general. Nothing else should be pointing to the RelocatablePtr (which in the actual example was inside of a struct), because if so the UniquePtr wouldn't be unique, so it can't get traced that way.

One way to describe the problem is that a UniquePtr is a pointer to a region of memory that's just as unsafe as the stack for storing unrooted pointers.
Blocks: GC.stability
I won't ask for review until this survives the try server.

I wonder what other types could be treated as pointing to unsafe memory?

Thinking through some other cases... if you have a UniquePtr<S>, and S contains a RootedSomething, then this will not report a hazard. Which is fine, though you should never have that code anyway (it breaks LIFO).

If S contains a PersistentRooted, then this will correctly not report a hazard.

If you have a struct S2 that contains a UniquePtr pointing to an unrooted pointer, this will correctly report a hazard.

It seems right to me, which is a sure sign that it's completely broken. Probably from a basic conceptual level. But we'll see.
I'll do another try push, but this is working locally for the shell build.
Attachment #8545327 - Flags: review?(terrence)
Attachment #8544822 - Attachment is obsolete: true
Comment on attachment 8545327 [details] [diff] [review]
Disallow storing unrooted pointer in memory pointed to be a UniquePtr

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

Nice!
Attachment #8545327 - Flags: review?(terrence) → review+
It *does* complain about a false positive in the following code:

        /* Construct the underlying C++ object. */
        auto dbg = cx->make_unique<Debugger>(cx, obj.get());
        if (!dbg || !dbg->init(cx))
            return false;

        Debugger *debugger = dbg.release();
        obj->setPrivate(debugger); // owns the released pointer

        ...something that can GC...

The concern is that you have a UniquePtr to a Debugger, which contains a field 'object' of type HeapPtr<NativeObject*> (basically a JSObject*). If you were to GC just before the dbg.release(), then nothing would trace Debugger.object and you'd potentially have a UAF or "UAM" (use-after-free or use-after-move).

In this case, it's a false positive because the UniquePtr hands off ownership with the dbg.release() call, so obj will trace it. I'm fixing by just wrapping the above chunk in { }. But it's pretty subtle; I certainly wouldn't spot a hazard if someone were to insert a GC call between the make_unique and the release().

Sadly, it does not catch any new real hazards. (I reverted the patch that triggered this bug, though, and it caught that.)
https://hg.mozilla.org/mozilla-central/rev/5dd6ef470908
https://hg.mozilla.org/mozilla-central/rev/575af71f0b24
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.