1480640 - False positives from ~RefPtr that won't go to zero

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

Thanks to fixing up some problems with destructor handling, I got the hazard pasted below.

The problem is the following code:

    RefPtr<JS::WasmModule> module = file.mWasmModule;

    JS::Rooted<JSObject*> wrappedModule(aCx, module->createObject(aCx));
    if (NS_WARN_IF(!wrappedModule)) {
      return nullptr;
    }

    result.set(wrappedModule);

    return result;

where 'result' is a Rooted<JSObject*>.

The hazard analysis sees that you're returning a JSObject* extracted from a Rooted<JSObject*>, but after you put it on the stack, ~RefPtr fires, possibly deleting the WasmModule and GCing in the process. (I don't know if that GC is actually possible, but it sure seems to run a scary amount of code.)

But in this case, I don't think the refcount can go to zero, though there's no simple way to prove it. file.mWasmModule has a reference to it, and 'file' seems very likely to survive past the end of this function call.

I'm not sure what I'm going to do with this hazard. Perhaps I should figure out how to prove that ~RefPtr<WasmModule> won't GC?

My other idea was using a RefPtrOfLongLivingThing<T>, though the easiest way to do that would be a simple T*, and I'm not sure how keen people would be about that.

The hazard:


Function '_ZN7mozilla3dom12_GLOBAL__N_134CopyingStructuredCloneReadCallbackEP9JSContextP23JSStructuredCloneReaderjjPv$IDBObjectStore.cpp:JSObject* mozilla::dom::{anonymous}::CopyingStructuredCloneReadCallback(JSContext*, JSStructuredCloneReader*, uint32, uint32, void*)' has unrooted '<returnvalue>' of type 'JSObject*' live across GC call '_ZN6RefPtrIN2JS10WasmModuleEED1Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule]' at dom/indexedDB/IDBObjectStore.cpp:1328
    IDBObjectStore.cpp:1328: Assign(139,140, return := __temp_46**)
    IDBObjectStore.cpp:1328: Call(140,141, wrappedModule.~Rooted())
    IDBObjectStore.cpp:1328: Call(141,142, module.~WasmModule>()) [[GC call]]
    IDBObjectStore.cpp:1328: Call(142,144, result.~Rooted())
    IDBObjectStore.cpp:1333:  [[end of function]]

GC Function: _ZN6RefPtrIN2JS10WasmModuleEED4Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule]
    static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModule; T = JS::WasmModule]
    static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModule]
    void js::AtomicRefCounted<T>::Release() const [with T = JS::WasmModule]
    Utility.h:void js_delete(JS::WasmModule*) [with T = JS::WasmModule]
    JS::WasmModule.__comp_dtor
    _ZN2js4wasm6ModuleD1Ev
    _ZN2js4wasm6ModuleD2Ev
    void js::wasm::Module::~Module(int32)
    _ZN2js21ExclusiveWaitableDataINS_4wasm7TieringEED1Ev
    _ZN2js21ExclusiveWaitableDataINS_4wasm7TieringEED2Ev
    js::ExclusiveWaitableData<js::wasm::Tiering>::~ExclusiveWaitableData()
    _ZN2js13ExclusiveDataINS_4wasm7TieringEED2Ev
    js::ExclusiveData<js::wasm::Tiering>::~ExclusiveData()
    _ZN2js4wasm7TieringD1Ev
    _ZN2js4wasm7TieringD2Ev
    void js::wasm::Tiering::~Tiering(int32)
    mozilla::Vector<T, N, AllocPolicy>::~Vector() [with T = RefPtr<JS::WasmModuleListener>; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]
    _ZN7mozilla6VectorI6RefPtrIN2JS18WasmModuleListenerEELm0EN2js17SystemAllocPolicyEED2Ev
    _ZN7mozilla6VectorI6RefPtrIN2JS18WasmModuleListenerEELm0EN2js17SystemAllocPolicyEED4Ev
    static void mozilla::detail::VectorImpl<T, N, AP, IsPod>::destroy(T*, T*) [with T = RefPtr<JS::WasmModuleListener>; long unsigned int N = 0ul; AP = js::SystemAllocPolicy; bool IsPod = false]
    RefPtr<T>::~RefPtr() [with T = JS::WasmModuleListener]
    _ZN6RefPtrIN2JS18WasmModuleListenerEED2Ev
    _ZN6RefPtrIN2JS18WasmModuleListenerEED4Ev
    static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener; T = JS::WasmModuleListener]
    static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener]
    JS::WasmModuleListener.Release
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Release()
    IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream()
    _ZN7mozilla3dom12_GLOBAL__N_124WasmCompiledModuleStreamD1Ev
    _ZN7mozilla3dom12_GLOBAL__N_124WasmCompiledModuleStreamD2Ev
    IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream(int32)
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Close()
    IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::CloseWithStatus(uint32)
    nsIInputStream.Close
    unresolved nsIInputStream.Close

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #0)
That's interesting.  Can we end up doing a CC (and hence GC) when we release a reference?  It does seem that this could cause us to run pretty much any code.

In this case, do we need to extract mWasmModule into a new separate RefPtr or can we just do file.mWasmModule->createObject(aCx)?  I'm assuming that can't clear file.mWasmModule or something.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> (In reply to Steve Fink [:sfink] [:s:] from comment #0)
> That's interesting.  Can we end up doing a CC (and hence GC) when we release
> a reference?  It does seem that this could cause us to run pretty much any
> code.
> 
> In this case, do we need to extract mWasmModule into a new separate RefPtr
> or can we just do file.mWasmModule->createObject(aCx)?  I'm assuming that
> can't clear file.mWasmModule or something.

That's a great question! And I don't know the answer. I don't understand anything about RefPtr.

But if it did clear file.mWasmModule, I don't think it would be a problem. Or rather, if it were a problem, it would report a hazard in the code that did the clear. As long as we get the JSObject* back successfully, there's nothing else that will trigger a GC after the return value is placed on the stack.

baku, is the right fix to do file.mWasmModule->createObject(aCx) here, or if not, is there another option?

Oh... I guess one way to do this would be just

    {
      RefPtr<JS::WasmModule> module = file.mWasmModule;

      JS::Rooted<JSObject*> wrappedModule(aCx, module->createObject(aCx));
      if (NS_WARN_IF(!wrappedModule)) {
        return nullptr;
      }

      result.set(wrappedModule);
    }

    return result;

Then ~RefPtr can GC all it wants while result is safely wrapped up in a Rooted. But I'm leaving the needinfo, as that seems simpler & cleaner if it's valid.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Fix hazard in CopyingStructuredCloneReadCallback

Better than a needinfo, here's a review request. It seems to compile, and fixes the two hazards.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac2b54e22a8
Fix hazard in CopyingStructuredCloneReadCallback, r=baku

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/8ac2b54e22a8

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8997576 [details] [diff] [review]
Fix hazard in CopyingStructuredCloneReadCallback

Approval Request Comment
[Feature/Bug causing the regression]: longstanding

[User impact if declined]: bug 1404274 is blocked from landing onto esr60. I don't know if that means we want to land this on beta to reduce the delta between beta and esr60?

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: it is in mozilla-central

[Needs manual test from QE? If yes, steps to reproduce]:  no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: it shifts around some operations slightly to remove a problematic series of actions that the rooting hazard analysis flags.

[String changes made/needed]: none



[esr60 Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed for bug 1404274 landing (to avoid a rooting hazard)

User impact if declined: see other bug. But this cleans up some sketchiness as well -- I don't see how to trigger it, but if something JS::WasmModule::createObject were to cause the WasmModule to self-destruct, then this is a theoretical UAF-style rooting hazard. Like I said, I don't see how that would be actually possible, but something could be added in the future or maybe there's an angle to this that I'm not seeing.

Fix Landed on Version: 63

Risk to taking this patch (and alternatives if risky): very low risk. It's just shifting some operations around a little.

String or UUID changes made by this patch: none

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8997576 [details] [diff] [review]
Fix hazard in CopyingStructuredCloneReadCallback

Let's take this for beta - sounds like a good idea to keep 62 in sync with ESR here even if the issue isn't showing up obviously in 62 beta.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/16a5f0d51b4a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997576 [details] [diff] [review]
Fix hazard in CopyingStructuredCloneReadCallback

Needed to uplift bug 1404274 to ESR60. Approved for ESR 60.2.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/bd80e86bf704