Closed Bug 929512 Opened 6 years ago Closed 6 years ago

Dictionaries inside unions should be optional

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 --- unaffected
firefox27 + fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: reuben, Assigned: bzbarsky)

References

Details

(Keywords: regression, sec-other, Whiteboard: [qa-])

Attachments

(4 files)

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?
Attachment #820373 - Flags: review?(bugs)
Could you please add tests.
I think updating TestInterface interface and its implementation's declaration is enough.
No longer blocks: 929287
Blocks: 929495
TestInterface has
void passUnion10(optional (EventInit or long) arg);
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 on attachment 820373 [details] [diff] [review]
Dictionaries in unions should be optional

There will be some a bit more complicated patch.
Attachment #820373 - Flags: review?(bugs)
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!
Group: core-security
And I don't know how I missed this when reading over the test codegen from bug 918011.  :(
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.
And in particular what's buggy is a non-nullable union containing a nullable type, with a default value of null.
Blocks: 918011
No longer depends on: 918011
Flags: needinfo?(bzbarsky)
Keywords: regression
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.
Flags: needinfo?(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.
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).
Assignee: nobody → bzbarsky
Attachment #820761 - Flags: review?(dzbarsky)
Attachment #820761 - Flags: review?(bugs)
(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 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)?
Attachment #820761 - Flags: review?(dzbarsky) → review+
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?
As far as I see we don't have any (non-test-only) interfaces using the problematic construct.
Keywords: sec-other
In that case, I'll just mark it sec-other.  Thanks for looking into this on your vacation, Boris!
> 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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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.
Attachment #820761 - Flags: review?(bugs) → review+
Is there any landing/uplift remaining here or can we call status-firefox27:fixed ?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
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.
I don't think there's anything QA needs to do here, no.
Thank you, Boris.
Whiteboard: [qa-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.