Closed Bug 631059 Opened 9 years ago Closed 9 years ago

Creating a FunctionType pointer instance from a JS function can fail

Categories

(Core :: js-ctypes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12

People

(Reporter: Unfocused, Assigned: Unfocused)

Details

(Whiteboard: [has patch])

Attachments

(1 file, 3 obsolete files)

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.
jorendorff, mrbkap, I bet you've got ideas here :)
Looks like the same basic problem as bug 624316, right?
Is this is simple as s/JS_ObjectIsFunction/JS_ObjectIsCallable/? i.e. will JS_CallFunctionValue work correctly on a wrapper?
Blair, you patch I'll review :)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #509647 - Flags: review?(dwitte)
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+
(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?
Yeah, the security wrapper is implemented as a proxy iiuc...
Every wrapper is a proxy these days, except "Wrapped Natives" which are really not wrappers (the new term de jour is "reflector").
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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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
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?
(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
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
(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
(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. ;)
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+
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?
Indeed. This is low risk and extension developers have been hitting this and flinging their arms up in shock and horror.
Attachment #510872 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/57157294fb52
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
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 → ---
Oh, lame. Blair, want to wrap that test in an if?
Test fixed, passed Try.
Attachment #510872 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [has patch][needs landing]
second try :)
http://hg.mozilla.org/mozilla-central/rev/51a4f02d7694
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.