929512 - Dictionaries inside unions should be optional

Status: RESOLVED FIXED

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

Description

[Reuben Morais [:reuben]]

Attachment: Dictionaries in unions should be optional

I wanted to have the following WebIDL:

[Constructor(optional (ContactProperties or mozContact) properties),
 JSImplementation="@mozilla.org/contact;1"]
interface mozContact { }

But then calling it like this:

 var c = new mozContact();

Fails with:

 TypeError: Argument 1 of mozContact.constructor could not be converted to any of: mozContact, ContactProperties.

When it should initialize an empty ContactProperties dictionary like it does if you have [Constructor(optional ContactProperties properties)].

I'm not sure my patch is the right way to fix this. With it, any union with a dictionary in it is automatically optional. This makes sense if you think that dictionaries are always optional, but at the same time it doesn't sound 100% right, because it means marking the union as optional is useless.

Olli, you worked on Bug 918011. What do you think?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Could you please add tests.
I think updating TestInterface interface and its implementation's declaration is enough.

Comment 2

[Reuben Morais [:reuben]]

TestInterface has
void passUnion10(optional (EventInit or long) arg);

Comment 3

[Reuben Morais [:reuben]]

TestInterface has:

 void passUnion10(optional (EventInit or long) arg);

So this bit of codegen is already being exercises, it just doesn't behave correctly.

Comment 4

[Reuben Morais [:reuben]]

Attachment: _construct without the patch

Comment 5

[Reuben Morais [:reuben]]

Attachment: _construct with the patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 820373 [details] [diff] [review]
Dictionaries in unions should be optional

There will be some a bit more complicated patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is bad.  If you look at the codegen for passUnion10, you get:

  EventInitOrLong arg0;
  EventInitOrLongArgument arg0_holder(arg0);
  {
    bool done = false, failed = false, tryNext;
    if (!done) {
      done = (failed = !arg0_holder.TrySetToEventInit(cx, args[0], args[0], tryNext)) || !tryNext;

which is totally wrong when args.length() == 0!

Comment 8

[Boris Zbarsky [:bzbarsky]]

And I don't know how I missed this when reading over the test codegen from bug 918011.  :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is actually a bug in the handling of null default values for unions.  This IDL also tickles it:

  void passUnion13(optional (object or long?) arg = null);

leading to this codegen:

  ObjectOrLongOrNullArgument arg0_holder(arg0);
  if (args[0].isNullOrUndefined()) {
    arg0_holder.SetNull();

which is bogus if args.length() == 0.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And in particular what's buggy is a non-nullable union containing a nullable type, with a default value of null.

Comment 11

[Daniel Veditz [:dveditz]]

Looks like we do have a couple of instances of this construct in our exposed .webidl (HTML option and select .add() for instance). Dunno if this is currently exploitable though.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> HTML option and select .add() for instance

If you just mean HTMLSelectElement.add(), that is NOT affected by this bug: it has a nullable union, not a union with a nullable member.

I just checked and we have no existing instances of the problematic pattern in our non-test WebIDL.  Not sure whether we want to keep this sec-sensitive, given that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Another example of test-only badness.  The init code for the (EventInit or long) member of the test Dict dictionary looks like this:


  {
    bool done = false, failed = false, tryNext;
    if (!done) {
      done = (failed = !mEventInitOrLong.TrySetToEventInit(cx, temp.ref(), &temp.ref(), tryNext)) || !tryNext;

but if isNull, then temp is not even constructed, so temp.ref() is garbage.

With the fix I'm about to post, this looks like so instead:

  if (!(!isNull && !temp.ref().isUndefined())) {
    if (!mEventInitOrLong.SetAsEventInit().Init(cx, JS::NullHandleValue, "Member of EventInitOrLong")) {
      return false;
    }
  } else {
    {
      bool done = false, failed = false, tryNext;
      if (!done) {
        done = (failed = !mEventInitOrLong.TrySetToEventInit(cx, temp.ref(), &temp.ref(), tryNext)) || !tryNext;

and the example from comment 9 becomes:

  if (!(args.hasDefined(0)) || args[0].isNullOrUndefined()) {
    arg0_holder.SetNull();
  } else {

while the example from comment 7 becomes:

  if (!(args.hasDefined(0))) {
    if (!arg0.SetAsEventInit().Init(cx, JS::NullHandleValue, "Member of EventInitOrLong")) {
      return false;
    }
  } else {
    {
      bool done = false, failed = false, tryNext;
      if (!done) {
        done = (failed = !arg0_holder.TrySetToEventInit(cx, args[0], args[0], tryNext)) || !tryNext;

so they all seem to be good.  And passUnion11 becomes:

  if (!(args.hasDefined(0))) {
    if (!arg0.SetAsCustomEventInit(cx).Init(cx, JS::NullHandleValue, "Member of CustomEventInitOrLong")) {
      return false;
    }
  } else {

etc (note the "cx" that needs to be passed there).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix null default values for non-nullable unions

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to On vacation Oct 12 - Oct 27 from comment #7)
> This is bad.  If you look at the codegen for passUnion10, you get:
Yup, we realized that with reuben and peterv yesterday.
I couldn't find any use of this construct so didn't see harm to keep the bug non-security sensitive.

Comment 16

[David Zbarsky (:dzbarsky)]

Comment on attachment 820761 [details] [diff] [review]
Fix null default values for non-nullable unions

Review of attachment 820761 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3129,5 @@
> +              isinstance(defaultValue, IDLNullValue)):
> +            assert type.hasDictionaryType
> +            assert defaultValue.type.isDictionary()
> +            if not isMember and typeNeedsRooting(defaultValue.type):
> +                ctorArgs = "cx"

Ah, I didn't know we started requiring this

@@ +3133,5 @@
> +                ctorArgs = "cx"
> +            else:
> +                ctorArgs = ""
> +            initDictionary = (
> +                'if (!%s.SetAs%s(%s).Init(cx, JS::NullHandleValue, "Member of %s")) {\n'

Why not use CGIfWrapper here?

::: dom/bindings/parser/WebIDL.py
@@ +2437,5 @@
>          if type.isUnion():
> +            # We use the flat member types here, because if we have a nullable
> +            # member type, or a nested union, we want the type the value
> +            # actually coerces to, not the nullable or nested union type.
> +            for subtype in type.unroll().flatMemberTypes:

I still think it's ridiculous that we treat (long or Dict?) differently than (long or Dict)?

Comment 17

[Andrew McCreight [:mccr8]]

What sort of rating should this get?  It seems like at best this is just reading uninitialized memory, but maybe we could be writing to uninitialized memory, too?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

As far as I see we don't have any (non-test-only) interfaces using the problematic construct.

Comment 19

[Andrew McCreight [:mccr8]]

In that case, I'll just mark it sec-other.  Thanks for looking into this on your vacation, Boris!

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Ah, I didn't know we started requiring this

That's needed because in that case our member is a RootedDictionary.

> Why not use CGIfWrapper here?

No good reason.  Fixed.

I checked in https://hg.mozilla.org/mozilla-central/rev/36fd56ac71dd with dzbarsky's review in the interests of not missing the merge, but would still appreciate smaug giving this a once-over when he gets the chance.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 820761 [details] [diff] [review]
Fix null default values for non-nullable unions

Generated code looked ok, though
if (!(0 < args.length())) { is a bit ugly.

Comment 22

[bhavana bajaj [:bajaj]]

Is there any landing/uplift remaining here or can we call status-firefox27:fixed ?

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Unless there is a test case or reproducible steps, QA is going to assume this can't be easily verified. Feel free to challenge that, though.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I don't think there's anything QA needs to do here, no.

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Thank you, Boris.