Bug 16545 – KJS::Bindings::Instance type conversions are not safe if multiple language bindings are used

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Michael Goddard]

If multiple language bindings (Qt, ObjC, JNI etc) are used at the same time, then there will be RuntimeObjectImps with KJS::Bindings::Instance * that are of multiple types (e.g. CInstance, QtInstance, JavaInstance).  

The type conversion code (JSObject to native object) always assumes that any RuntimeObjectImp objects are of the same language binding as the conversion code and can downcast the Instance* to the wrong type, possibly causing crashes and memory corruption.  For example, binding a Qt object to "foo", a Java object to "bar", and calling a method like "bar.method(foo)" would cast the QtInstance* for foo to a JavaInstance*.

[Michael Goddard]

I've not actually tested this, however, but it is apparent from code inspection...

[Michael Goddard]

Created attachment 18021 [details]
Concept patch

Concept patch for this issue.  Qt changes are missing, because I'm in the process of updating them separately.  I don't believe the original Qt code had the conversion code, however.

I've not compiled the JNI or ObjC changes either - I lack the platform :(

[Michael Goddard]

Created attachment 18231 [details]
Slightly different patch - don't bother returning Instance* 

For simpler code, don't return the Instance* when all anyone ever does with it is to return the native void* that it wrapped.  This does add a void* member to Instance, but this is useful for more than just this purpose (I have a patch for caching JSObject/Instance * for the same native object*, for example).

[Geoffrey Garen]

This patch is challenging to review because it doesn't include a test to verify its correctness. The bindings code is fairly brittle -- should we really be making changes like this without a measurable benefit? Does the Qt port need this for something?

[Michael Goddard]

Mostly this patch is for theoretical safety (in the case where somebody adds both JNI/C objects and Qt objects to a page, and calls one with the other).  I do have some testcases for the Qt bindings, but they aren't really changed by this patch.

We can leave this issue for now, I think.

[Darin Adler]

Comment on attachment 18231 [details]
Slightly different patch - don't bother returning Instance* 

Looks good. Thanks for tackling this.

We really need a way to test this! We require tests that demonstrate bugs when we fix them. Let figure out a way to come up with a test case.

The additional void* pointer is unnecessary. I agree that we need to check the binding language, but this particular idiom, moving the pointers up into the shared base class, doesn't seem good to me. I don't think it really makes things all that much simpler. Here's what I suggest:

+        NPObject* instance = static_cast<NPObject *>(Instance::extractNativeInstanceFromObject(object, Instance::CLanguage));

Lines of code like the above, should be simply:

    NPObject* instance = getNPObject(object);

We can write the function getNPObject like this:

    static Instance* getInstance(JSObject* object, BindingLanguage language)
    {
        if (!object)
            return 0;
        if (!object->inherits(&RuntimeObjectImp::info))
            return 0;
        Instance* instance = static_cast<RuntimeObjectImp*>(object);
        if (instance->bindingLanguage() != language)
            return 0;
        return instance;
    }

    static CInstance getCInstance(JSObject* object)
    {
        return static_cast<CInstance>(getInstance(object, CLanguage));
    }

    static NPObject* getNPObject(JSObject* object)
    {
        CInstance* instance = getCInstance(object);
        if (!instance)
            return 0;
        return instance->getObject();
    }

We can put these functions wherever we think is appropriate. They can be static member functions or free functions or whatever. The functions that have to be written per-language are very simple -- boilerplate that's only a couple lines long. If we really thought it necessary we could even make them use templates so we don't have to write them multiple times. But I see no real benefit to adding an additional field storing a redundant copy of the binding object pointer.

Smaller comments that may be obviated by the above:

-    : Instance(rootObject)
+    : Instance((void*) o, rootObject)

-    : Instance(rootObject)
+    : Instance((void*) instance, rootObject)

-    : Instance(rootObject),
+    : Instance((void*) o, rootObject),

The type void* is a sort of "universal recipient", so there's no need to typecast at sites like these.

+/*!
+   If the supplied \a object is a runtime object for a native object of the specified
+   \a instanceLanguage, return the corresponding native object pointer as a void*.  If the \a object is not
+   a runtime object, or is not of the specified \a instanceLanguage, returns 0.
+*/

We don't use this syntax for comments. I assume it's Doxygen or something like that. It doesn't make sense to have this one comment with a different formatting and the \a characters in it in a project where none of the other comments do. Unless we have a plan of doing this sort of thing going forward across the project.

+    static void* extractNativeInstanceFromObject(JSObject* object, BindingLanguage instanceLanguage);

In declarations like this, we omit the argument names if their types make them unambiguous.

[Alexey Proskuryakov]

This was rediscovered later, and fixed in a different way.

*** This bug has been marked as a duplicate of bug 35394 ***