631059 - Creating a FunctionType pointer instance from a JS function can fail

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[[e-mail]]

jorendorff, mrbkap, I bet you've got ideas here :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like the same basic problem as bug 624316, right?

Comment 3

[[e-mail]]

Is this is simple as s/JS_ObjectIsFunction/JS_ObjectIsCallable/? i.e. will JS_CallFunctionValue work correctly on a wrapper?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Yes and yes.

Comment 5

[[e-mail]]

Blair, you patch I'll review :)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

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

[[e-mail]]

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.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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

[[e-mail]]

Yeah, the security wrapper is implemented as a proxy iiuc...

Comment 10

[Andreas Gal :gal]

Every wrapper is a proxy these days, except "Wrapped Natives" which are really not wrappers (the new term de jour is "reflector").

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

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?

Comment 13

[[e-mail]]

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

[Brendan Eich [:brendan]]

(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

[[e-mail]]

(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

[[e-mail]]

Attachment: patch v2

This just removes the relevant tests, and tweaks your additional one so it passes. :)

Look OK?

Comment 17

[Brendan Eich [:brendan]]

(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

[[e-mail]]

(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 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 21

[[e-mail]]

Indeed. This is low risk and extension developers have been hitting this and flinging their arms up in shock and horror.

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/57157294fb52

Comment 23

[Marco Bonardo [:mak]]

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

Comment 24

[[e-mail]]

Oh, lame. Blair, want to wrap that test in an if?

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1 - ready for checkin

Test fixed, passed Try.

Comment 26

[Marco Bonardo [:mak]]

second try :)
http://hg.mozilla.org/mozilla-central/rev/51a4f02d7694