Bug 101190 – [V8] Remove setDOMWrapper(wrapper, 0) from V8NPObject

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

In bug 101054, we want to add an ASSERT() that guarantees that we don't pass 0 to setDOMWrapper(). On the other hand, V8NPObject.cpp is trying to pass 0 to setDOMWrapper() just before disposing a wrapper, which hits the ASSERT(). We should remove it.

[Kentaro Hara]

Created attachment 172284 [details]
Patch

[Adam Barth]

Comment on attachment 172284 [details]
Patch

What if other folks have a reference to the v8 object?  After this change, won't they have a use-after-free if they try to access the wrapped object?

In the DOM case, we only remove objects from the wrapper map when we get a weak handle callback.  In the case of NP objects, we can remove them when asked to do so explicitly by the plugin (you might want to check that I'm right about that---it's from memory).

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 172284 [details])
> What if other folks have a reference to the v8 object?  After this change, won't they have a use-after-free if they try to access the wrapped object?

Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it?

[Adam Barth]

> Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it?

No, that just means that we don't have a reference anymore.  There could be other handles to the object, either local or persistent or internal to the JS VM.

[Kentaro Hara]

(In reply to comment #4)
> > Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it?
> 
> No, that just means that we don't have a reference anymore.  There could be other handles to the object, either local or persistent or internal to the JS VM.

Yeah, makes sense... but that sounds like another problem. In bindings (even in V8NPObject.cpp), we do not do a null check for the result of toNative(handle). If there were other folks holding a handle to the wrapper, the folks might retrieve null pointers from internal fields and use them.

[Adam Barth]

I think only NP objects need to null-check toNative.

[Kentaro Hara]

(In reply to comment #6)
> I think only NP objects need to null-check toNative.

Thanks, now I understood things.

As far as I scanned the code, the only place where toNative(npObjectWrapper) is called is weakNPObjectCallback() in V8NPObject.cpp. Just after toNative(npObjectWrapper), we have an ASSERT() that checks if the retrieved npObject is not NULL. So, the world is working safely.

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 172363 [details]
Patch

[Adam Barth]

Comment on attachment 172363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172363&action=review

> Source/WebCore/bindings/v8/V8DOMWrapper.h:77
> +        static void resetDOMWrapper(v8::Handle<v8::Object> object, WrapperTypeInfo* type)

resetDOMWrapper -> clearDOMWrapper?

[Kentaro Hara]

Created attachment 172519 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 172519 [details]
patch for landing

Rejecting attachment 172519 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14690390

[Kentaro Hara]

Created attachment 172542 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 172542 [details]
patch for landing

Clearing flags on attachment: 172542

Committed r133589: <http://trac.webkit.org/changeset/133589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.