Closed
Bug 631059
Opened 14 years ago
Closed 14 years ago
Creating a FunctionType pointer instance from a JS function can fail
Categories
(Core :: js-ctypes, defect)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
mozilla2.0b12
People
(Reporter: Unfocused, Assigned: Unfocused)
Details
(Whiteboard: [has patch])
Attachments
(1 file, 3 obsolete files)
4.30 KB,
patch
|
Details | Diff | Splinter Review |
Given the following code:
var FuncPtr = ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, [ctypes.voidptr_t, ctypes.voidptr_t, ctypes.uint32_t]).ptr;
function myFunc(a, b, c) { .. }
var f_ptr = FuncPtr(myFunc);
The last line will throw with:
Error: expected type pointer, got function myFunc(a, b, c) {...}
The check here is failing:
https://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#3287
Specifically, the call to JS_ObjectIsFunction() - because the object passed in is a function proxy, when it's expecting to see a function.
Comment 1•14 years ago
|
||
jorendorff, mrbkap, I bet you've got ideas here :)
![]() |
||
Comment 2•14 years ago
|
||
Looks like the same basic problem as bug 624316, right?
Comment 3•14 years ago
|
||
Is this is simple as s/JS_ObjectIsFunction/JS_ObjectIsCallable/? i.e. will JS_CallFunctionValue work correctly on a wrapper?
Comment 4•14 years ago
|
||
Yes and yes.
Comment 5•14 years ago
|
||
Blair, you patch I'll review :)
Assignee | ||
Comment 6•14 years ago
|
||
1-line patches don't normally take me this long, really. (Was off sick)
Also: No idea how to test that, given the existing tests cover FunctionTypes, and didn't catch this.
Comment 7•14 years ago
|
||
Comment on attachment 509647 [details] [diff] [review]
Patch v1
r=dwitte if it makes your testcase work :)
Also ummm, ask mrbkap how you could test? I assume there's some way to proxify a JS function. Which should be sufficient.
Attachment #509647 -
Flags: review?(dwitte) → review+
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> I assume there's some way to proxify a JS function. Which should be sufficient.
My usecase is a restartless extension, so the function is in a sandbox - is that why I got a function proxy?
Comment 9•14 years ago
|
||
Yeah, the security wrapper is implemented as a proxy iiuc...
Comment 10•14 years ago
|
||
Every wrapper is a proxy these days, except "Wrapped Natives" which are really not wrappers (the new term de jour is "reflector").
Comment 11•14 years ago
|
||
Yeah, the easiest way to get your hands on a proxy is to create a sandbox (it doesn't matter what principal) and get a function out of it.
Assignee | ||
Comment 12•14 years ago
|
||
Dan: I was sure I ran the tests for this earlier... but its failing in the explicit conversion tests:
let f2_t = ctypes.FunctionType(ctypes.default_abi, g_t, [ ctypes.int32_t ]);
let f2 = f2_t.ptr();
do_check_throws(function() { fp_t(f2); }, Error);
No exception is thrown there.
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/ctypes/tests/unit/test_jsctypes.js.in#1864
I'm way out of my depth here, so can I pass this off to you?
Attachment #509647 -
Attachment is obsolete: true
Comment 13•14 years ago
|
||
fp_t is a FunctionType.ptr like so:
let g_t = ctypes.StructType("g_t", [{ a: ctypes.int32_t }, { b: ctypes.double }]);
let f_t = ctypes.FunctionType(ctypes.default_abi, g_t);
let fp_t = f_t.ptr;
And f2_t is a FunctionType.ptr of a different type:
// Test ExplicitConvert from a function pointer of different type.
let f2_t = ctypes.FunctionType(ctypes.default_abi, g_t, [ ctypes.int32_t ]);
let f2 = f2_t.ptr();
do_check_throws(function() { fp_t(f2); }, Error);
So constructing a function of type fp_t from f2 (in this case, a NULL function pointer, but that's ancillary) should fail, since fp_t != f2_t. This happens via ImplicitConvert, which will refuse to convert this case.
So the question is, why would JS_ObjectIsCallable() return true for a FunctionType.ptr instance? I'm guessing it's because the object has a call hook: http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#276
Sooo... how can one distinguish between things we want (actual JS functions and their proxies) and things we don't (native code)? Maybe this is a distinction we no longer need to make.
For a native function, will JS_ValueToObject --> JS_CallFunctionValue work? Or crash horribly?
Comment 14•14 years ago
|
||
(In reply to comment #13)
> So the question is, why would JS_ObjectIsCallable() return true for a
> FunctionType.ptr instance? I'm guessing it's because the object has a call
> hook:
> http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#276
>
> Sooo... how can one distinguish between things we want (actual JS functions and
> their proxies) and things we don't (native code)? Maybe this is a distinction
> we no longer need to make.
If you need to care, typeof x == "function" or JS_TypeOfValue is the way.
> For a native function, will JS_ValueToObject --> JS_CallFunctionValue work?
Of course.
/be
Comment 15•14 years ago
|
||
(In reply to comment #14)
> If you need to care, typeof x == "function" or JS_TypeOfValue is the way.
Hmm. JS_TypeOfValue appears to use obj->isCallable() to determine whether it's a function, which leads us to the same problem; we have no idea if the underlying object is actually a JS function or a JSClass with a call hook. And since I'm guessing proxies are the latter anyway, it's not a sensible question to ask.
So then we're stuck with allowing the behavior. Perhaps we could aggressively check the class of the object and do strict type checking if it's another FunctionType.ptr instance, but that seems a little pointless if you can pass in any other callable you manage to get your hands on from script.
Comment 16•14 years ago
|
||
This just removes the relevant tests, and tweaks your additional one so it passes. :)
Look OK?
Attachment #510186 -
Attachment is obsolete: true
Attachment #510872 -
Flags: review?(bmcbride)
Comment 17•14 years ago
|
||
(In reply to comment #15)
> (In reply to comment #14)
> > If you need to care, typeof x == "function" or JS_TypeOfValue is the way.
>
> Hmm. JS_TypeOfValue appears to use obj->isCallable() to determine whether it's
> a function, which leads us to the same problem; we have no idea if the
> underlying object is actually a JS function or a JSClass with a call hook. And
> since I'm guessing proxies are the latter anyway, it's not a sensible question
> to ask.
You could (if you needed to) implement a callable object whose typeof-type is not "function", by implementing ObjectOps and hooking its typeOf member.
> So then we're stuck with allowing the behavior. Perhaps we could aggressively
> check the class of the object and do strict type checking if it's another
> FunctionType.ptr instance, but that seems a little pointless if you can pass in
> any other callable you manage to get your hands on from script.
Yeah. The behavior is not a bug (AFAICT), just a kind of non-feature. Right?
/be
Comment 18•14 years ago
|
||
(In reply to comment #17)
> Yeah. The behavior is not a bug (AFAICT), just a kind of non-feature. Right?
Yup. The case we're trying to prevent isn't really one that people should run into; I suppose it was somewhat of a misguided attempt to safety a footgun, but we have enough of those already. ;)
Assignee | ||
Comment 19•14 years ago
|
||
Comment on attachment 510872 [details] [diff] [review]
patch v2
Huh, I actually thought that might be the case, and dismissed it, since I figured it seemed too simple. That'll teach me for second-guessing myself.
Attachment #510872 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 20•14 years ago
|
||
Comment on attachment 510872 [details] [diff] [review]
patch v2
From my understanding, this is relatively low risk. The main benefit is that ctypes.FunctionType will now work in restartless extensions.
Attachment #510872 -
Flags: approval2.0?
Comment 21•14 years ago
|
||
Indeed. This is low risk and extension developers have been hitting this and flinging their arms up in shock and horror.
Updated•14 years ago
|
Attachment #510872 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 22•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Comment 23•14 years ago
|
||
backed out due to m-oth failure
http://hg.mozilla.org/mozilla-central/rev/447d780336cc
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297437972.1297439177.23974.gz
7741 INFO TEST-START | chrome://mochitests/content/chrome/toolkit/components/toolkit/components/ctypes/tests/test_ctypes.xul
7742 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/toolkit/components/ctypes/tests/test_ctypes.xul | Chrome test dir doesn't exist?!
7743 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/toolkit/components/ctypes/tests/test_ctypes.xul | ctypes test library doesn't exist!?
7744 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/toolkit/components/ctypes/tests/test_ctypes.xul | Worker had an error: Components.classes['@mozilla.org/systemprincipal;1'] is undefined
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•14 years ago
|
||
Oh, lame. Blair, want to wrap that test in an if?
Assignee | ||
Comment 25•14 years ago
|
||
Test fixed, passed Try.
Attachment #510872 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][needs landing]
Comment 26•14 years ago
|
||
second try :)
http://hg.mozilla.org/mozilla-central/rev/51a4f02d7694
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Flags: in-litmus-
Whiteboard: [has patch][needs landing] → [has patch]
You need to log in
before you can comment on or make changes to this bug.
Description
•