1369533 - Callback-to-JS conversions can generally produce null

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

After bug 1273251, callback-to-JS conversions can generally produce null.

We have a few options for handling this:

1)  Disallow to-JS conversion for non-nullable callbacks.  For nullable callbacks, things will already work correctly once bug 1369367 is fixed.

2)  Allow non-nullable callbacks to still convert to null.  This will require at least the following things to be fixed:

  * getJSReturnTypeTag for callbacks.  Right now we claim to the jit that a callback return type will never be null.  This is why this is security-sensitive.

  * getMaybeWrapValueFuncForType for callbacks; it will need to return MaybeWrapObjectOrNullValue

but maybe there are others too.  This is a lot more fragile than option 1, because we have to go through and audit all the various places that might depend on the Value produced from a callback not being null.

Option 1, of course, has the benefit of not needing to deviate from spec IDL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Gah.  Bugzilla nicely lost my comment...

Summary: Option 1 is not viable because there are specced APIs (e.g. a bunch of webrtc stuff) that use non-nullable callbacks, and making them nullable is observably not spec-compliant.

Also, the consumers we have right now that do getJSReturnTypeTag on a non-nullable callback are either test-only or chromeonly, so the security exposure is somewhat mitigated...

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression from 53. From the discussion in bug 1369367 sounds like 54 and 55 are also affected.

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

I guess the real question is who will do the auditing we need for approach 2 above.  I can do it next week if no one gets to it before that....

Comment 4

[Ryan VanderMeulen [:RyanVM]]

We're spinning the Fx54 RC build today, so this isn't going to make that release.

Comment 5

[Kris Maglione [:kmag]]

There's also another option:

3) Replace the callback with a dead wrapper rather than setting it to null.

That would give us more or less the same behavior as we have most other places, where we explicitly wrap the value for the compartment of the object we're storing it on.

But my preference would be to disallow storing callback functions that might eventually be nuked in any location where the value would be accessible. The most obvious example is an extension setting an event listener property on a content document, which aside from having namespacing conflicts (until bug 1330113 is fixed), gives the content scripts access to it.

But maybe there are other places where there's no viable workaround.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 3) Replace the callback with a dead wrapper rather than setting it to null.

Hmm.  Is there a reason we didn't do that to start with, instead of changing invariants?

> my preference would be to disallow storing callback functions that might eventually
> be nuked in any location where the value would be accessible.

That's pretty hard to do, because determining "would be accessible" is a pretty non-local decision.  How would addEventListener() be able to tell that the value is not "accessible"?  The fact that it's not is kind of an implementation detail of EventListenerManager::GetListenerInfo, right?

Did you have a concrete proposal for how this would work?

Comment 7

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #6)
> > 3) Replace the callback with a dead wrapper rather than setting it to null.
> 
> Hmm.  Is there a reason we didn't do that to start with, instead of changing
> invariants?

Well, mainly because I went with the implementation that you suggested. But also because smaug was working on a web-facing version of this with similar semantics, and because it's easier said than done. But it should be doable.

> > my preference would be to disallow storing callback functions that might eventually
> > be nuked in any location where the value would be accessible.
> 
> That's pretty hard to do, because determining "would be accessible" is a
> pretty non-local decision.  How would addEventListener() be able to tell
> that the value is not "accessible"?  The fact that it's not is kind of an
> implementation detail of EventListenerManager::GetListenerInfo, right?
> 
> Did you have a concrete proposal for how this would work?

No. I think it would probably have to be done on a case-by-case basis, unfortunately.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  If there's any complication at all in doing option 3, I think we should just do option 2.  It just needs auditing of codegen for all the isCallback and isCallbackInterface bits and changing them all accordingly.  Pretty straightforward; just tedious.

Comment 9

[Kris Maglione [:kmag]]

It should be easier since bug 1354733, which lets us create dead wrappers directly when we don't have a wrapper to nuke. If we want the dead wrapper to report itself as callable, though, it will need a bit more work.

After that, I think we should be able to change the deferred finalizer to null out the incumbent global and replace the JS callback with a dead wrapper, rather than nulling it out and calling DropJSObjects.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I don't think we care about the dead wrapper reporting itself as callable.

Comment 11

[Gerry Chang [:gchang]]

Track 54- as we've already built 54 RC and the security exposure is somewhat mitigated according to comment #1. Feel free to nominate again if the security level comes to critical/high.

Comment 12

[Andrew McCreight [:mccr8]]

I'm marking this audit because it sounds like you aren't sure if there's an issue. Please clear the keyword or adjust the rating if that is incorrect.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10)
> I don't think we care about the dead wrapper reporting itself as callable.

I wouldn't think so either, but based on bug 1354294, I'm not sure.

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #13)
> (In reply to Boris Zbarsky [:bz] (if a patch has no decent message,
> automatic r-) from comment #10)
> > I don't think we care about the dead wrapper reporting itself as callable.
> 
> I wouldn't think so either, but based on bug 1354294, I'm not sure.

We don't care whether a dead wrapper is callable, but we do care about it preserving the callability. (For context: I ran into crashes from bug 1354294 when I tried to make dead wrappers always callable, even if the previously live thing was not callable. Since it's more semantically intuitive that dead wrappers don't change IsCallable and IsConstructor, I opted to not spend the time debugging and preserve those properties.)

The current behavior is when constructing a DeadObjectProxy, it retains the constructibility and callability of the nuked target.

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> It should be easier since bug 1354733, which lets us create dead wrappers
> directly when we don't have a wrapper to nuke. If we want the dead wrapper
> to report itself as callable, though, it will need a bit more work.
> 

You can get a callable DeadProxyObject handler directly with |DeadObjectProxy<DeadProxyIsCallable{Not,Is}Constructor>::singleton()|. Does that help?

Comment 15

[Kris Maglione [:kmag]]

(In reply to Shu-yu Guo [:shu] from comment #14)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment
> #13)
> > (In reply to Boris Zbarsky [:bz] (if a patch has no decent message,
> > automatic r-) from comment #10)
> > > I don't think we care about the dead wrapper reporting itself as callable.
> > 
> > I wouldn't think so either, but based on bug 1354294, I'm not sure.
> 
> We don't care whether a dead wrapper is callable, but we do care about it
> preserving the callability.

That probably doesn't matter for this case, then. We'd be creating a new wrapper entirely, and no code in the JS engine should expect the getter to return the same object, or the same type of object, that it returned previously.

Although... that might change if we wind up marking any of those attributes Pure at some point.

> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> You can get a callable DeadProxyObject handler directly with
> |DeadObjectProxy<DeadProxyIsCallable{Not,Is}Constructor>::singleton()|. Does
> that help?

The only real issue at this point is that those are non-public APIs, and the only jsfriend API for creating new dead wrappers can only create them based on an existing wrapper, or as non-callable/non-constructable. It probably wouldn't be too hard to change that to allow explicitly specifying those properties, or to get them from a non-wrapper object, but it would be much easier not to.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you intend to make changes for the 55 or 56 timeframe? 
If so, can you help figure out who's actually going to take the bug?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Either me or Kris, depending on the solution we pick.  Kris, do you think you can do #3 reasonably?  If not, I will make the time to go through and do #2...

Comment 18

[Kris Maglione [:kmag]]

Yes, I think #3 is doable at this point.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Kris, are you still planning to get to this?  It looks like it missed 55 now...

Comment 20

[Kris Maglione [:kmag]]

Yes. Sorry, other urgent bugs came up in the mean time. I'll put it on my todo list for this week.

Comment 21

[Kris Maglione [:kmag]]

Attachment: Return dead wrappers rather than null for dead CallbackObject values

Boris, are you willing to review this?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8904383 [details] [diff] [review]
Return dead wrappers rather than null for dead CallbackObject values

This looks good as far as bindings go, thank you!

I think we still have a few places with not-obviously-ok CallbackOrNull/CallableOrNull bits:

1) CustomElementRegistry::Define will crash if the callable is null.
   Can it not be null there?
2) CustomElementRegistry::Get should arguably hand back a dead object
   wrapper, not null.
3) DOMEventTargetHelper::GetEventHandler should arguably hand back
   a dead object wrapper.
4) I dunno whether EventListenerInfo::GetJSVal is even used...
5) The CallbackObject overload of ToJSValue over in ToJSValue.h should
   hand back a dead object, I'd think.

Please make sure a followup or followups is reported on those, and let me know whether I should pick the followups up or whether you have the time.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 56 at this point as the release will be next week. 
kmag, bz, if you want to land this for 57, it should be soon.

Comment 24

[Kris Maglione [:kmag]]

It's still on my priority list for this week. I would have had it done yesterday, but I had to spend half the day recovering from a power failure that outlasted my UPS and laptop batteries :(

Comment 25

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #22)
> I think we still have a few places with not-obviously-ok
> CallbackOrNull/CallableOrNull bits:
>
> 1) CustomElementRegistry::Define will crash if the callable is null.
>    Can it not be null there?

It can't, though possibly for non-obvious reasons. The only way a callback
object can ever wind up being null is if we nuke the compartment it belongs to,
and then run the cycle collector's forget-skippable phase, and it calls the
CallbackObject's CanSkip.

Normally the second part is prevented in binding methods by the fact that we use
RootedCallback<>s, which means the CC never sees them before the binding method
does something with them.

In this case, though, there's just no opportunity for code to run that could
nuke the owner compartment or trigger the cycle collector. Perhaps some comments
in nsDocument::RegisterElement and nsDocument::RegisterElement to ensure that
continues to be the case would make sense.

> 2) CustomElementRegistry::Get should arguably hand back a dead object
>    wrapper, not null.
> 3) DOMEventTargetHelper::GetEventHandler should arguably hand back
>    a dead object wrapper.

Hm. Quite. I'd have to find a place where we still use XPIDL event targets to
test that...

> 4) I dunno whether EventListenerInfo::GetJSVal is even used...

It is. I'm not sure why Searchfox doesn't index it properly.

The event listener service filters out dead callbacks when it's asked to iterate
event listeners, though, so this isn't an issue.

Comment 26

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> > 3) DOMEventTargetHelper::GetEventHandler should arguably hand back
> >    a dead object wrapper.
> 
> Hm. Quite. I'd have to find a place where we still use XPIDL event targets to
> test that...

It seems like everywhere we currently use NS_IMPL_EVENT_HANDLER, we're already using WebIDL bindings. So we should probably just remove the XPIDL versions. But that's really another bug. For now, I'll just make the change but not add any tests for it.

Comment 27

[Kris Maglione [:kmag]]

Attachment: Return dead wrappers rather than null for dead CallbackObject values

Comment 28

[Boris Zbarsky [:bzbarsky]]

> For now, I'll just make the change but not add any tests for it.

Sounds good.  I filed bug 1401848 on removing NS_IMPL_EVENT_HANDLER.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8910515 [details] [diff] [review]
Return dead wrappers rather than null for dead CallbackObject values

r=me

Comment 30

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8af47160570c05088128428bae1eb69bf48b0dc4
Bug 1369533: Return dead wrappers rather than null for dead CallbackObject values. r=bz

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8af47160570c