Summary: | KJS::Bindings::Instance type conversions are not safe if multiple language bindings are used | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Goddard <michael.goddard> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED DUPLICATE | ||||||||
Severity: | Normal | CC: | ap, ggaren | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Michael Goddard
2007-12-20 17:19:49 PST
I've not actually tested this, however, but it is apparent from code inspection... 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 :(
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).
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? 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. 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.
|