714614 - Assertion failure: self->nativeContains(cx, *aprop), at jsscope.cpp:1000

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision d98fbf3cbd71 (options -m -a -n):


function testForVarInWith(foo, foo) {
  return eval("with ({}) { for (var x = 0; x < 5; x++); } (function() { return delete x; })");
}
f = testForVarInWith()();

Comment 1

[Brian Hackett [Laid off!]]

The duplicate parameter names end up causing bindings and call objects to be created with multiple shapes with the same id, which is a pretty big object layout invariant violation (and one which existed before objshrink).  This doesn't fail with a pre-objshrink tree, but that is only because the 'delete x' converts the call object to a dictionary instead of unwinding the scope (necessary because the object flags are different between the last and previous properties).

s-s because I don't know how far back this goes, and accesses on the call objects could behave incorrectly and break e.g. TI information.

Comment 2

[Johnny Stenback (:jst)]

Dave, can you assign this appropriately?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   81298:33962bb21403
user:        Brian Hackett
date:        Tue Nov 08 16:56:00 2011 -0800
summary:     Set DELEGATE for parents of other objects, bug 700300.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #3)
> autoBisect shows this is probably related to the following changeset:

No, this isn't, see comment 1.

Comment 5

[David Mandelin [:dmandelin]]

This won't be fixed in time for 10.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: qnd fix

Quick and dirty fix.

The problem is that the bindings object gets two properties with the same name, which is sketchy but probably OK in itself--but that causes the Call objects to get two properties with the same name, which is bad. The real fix (which is planned for some point in the future) is to use a special name mapping object for bindings and call objects. 

To close the vulnerability for now, this fix instead just doesn't create a duplicate-named property in the bindings object. For the most part, that just works, except (yay) the decompiler, which uses the bindings to print the parameters. For now, we just flag the object as having holes, and fill in the parameter names with fake values in the decompiler.

If that turns out not to be good enough, we could store a list of duplicate names on the side, but that bloats scripts by a word and would be generally a pain, which doesn't seem worth it for such an obscure use case (recompilation of string representations of functions with duplicate parameter names).

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 592306 [details] [diff] [review]
qnd fix

>+                     * Hack: see comment in Bindings::hasHoles.

This can be a single-line comment

>+    if (hasHoles) {
>+        AutoKeepAtoms keep(cx->runtime);
>+        const char *dupchars = "<duplicate name>";
>+        JSAtom *dup = js_Atomize(cx, dupchars, strlen(dupchars));
>+        if (!dup)
>+            return false;
>+        for (unsigned i = 0; i < n; i++)
>+            names[i] = dup;
>+    }

I don't think AutoKeepAtoms is necessary, since there is only once GC allocation in its scope.  Second, I don't think its sufficient since the atom is used after we leave the scope of 'keep'.  Instead, why don't you just pass js::InternAtom to js_Atomize, effectively rooting the atom for life.

Comment 8

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/602d30678cac

Comment 9

[David Mandelin [:dmandelin]]

That patch got backed out. It hit some failures in JSD tests that want to get the duplicate parameter names back from the decompiler, for which we could just change the tests, but there's a more serious problem:

  function f(x,x) { "use strict" };

That's a syntax error, and the error message should be able to give us the name of the duplicate parameter. In order for that to work, we do need to record full information about the duplicate parameters in some form or another inside Bindings, and we have to do it even if we don't know the function is in strict mode while parsing arguments.

One idea would be to have two shapes in the binding: one that has duplicate parameters for decompilation and strict mode checking and another that omits them for creating Call objects. But it seems wasteful to create an entire extra copy of the shapes all the time given that duplicate parameters should be rare.

The other idea I had earlier (but didn't want to bother with) was to have an optional array of information about duplicates. So the shapes would omit duplicates, but there would be an optional array on the side saying which parameters are duplicates of which other parameters, and that could be used by duplicates and the strict mode checks.

Comment 10

[Brian Hackett [Laid off!]]

It might work to tolerate the duplicates just in the script's bindings, but when creating call objects make sure that duplicates are filtered out and the resulting object has a valid no-duplicate shape lineage.  JSScript can just have a bit indicating whether there are any duplicate names in the bindings, and if this bit is found then call object creation takes a slow path.

Comment 11

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> It might work to tolerate the duplicates just in the script's bindings, but
> when creating call objects make sure that duplicates are filtered out and
> the resulting object has a valid no-duplicate shape lineage.  JSScript can
> just have a bit indicating whether there are any duplicate names in the
> bindings, and if this bit is found then call object creation takes a slow
> path.

Yes, I think that's good, I was just thinking the same thing while waking up. It even allows the decompiler to work correctly as long as no call objects have been created yet. Wheee...

Comment 12

[David Mandelin [:dmandelin]]

(In reply to David Mandelin from comment #11)
> (In reply to Brian Hackett (:bhackett) from comment #10)
> > It might work to tolerate the duplicates just in the script's bindings, but
> > when creating call objects make sure that duplicates are filtered out and
> > the resulting object has a valid no-duplicate shape lineage.  JSScript can
> > just have a bit indicating whether there are any duplicate names in the
> > bindings, and if this bit is found then call object creation takes a slow
> > path.
> 
> Yes, I think that's good, I was just thinking the same thing while waking
> up. It even allows the decompiler to work correctly as long as no call
> objects have been created yet. Wheee...

Or I guess you probably meant the slow path creates the call object without the dups while still keeping the dups within the binding, so the decompiler always works correctly. So I think that's correct always, and the only cost is that creating call objects is slow if there are dup arguments, which should not matter at all.

Comment 13

[Brian Hackett [Laid off!]]

(In reply to David Mandelin from comment #12)
> Or I guess you probably meant the slow path creates the call object without
> the dups while still keeping the dups within the binding, so the decompiler
> always works correctly. So I think that's correct always, and the only cost
> is that creating call objects is slow if there are dup arguments, which
> should not matter at all.

Yeah, the bindings should keep any duplicate names after any no-duplicate call objects have been created.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: WIP

Doesn't work yet--crashes in getChildBinding probably due to using it wrong or something. Also I'm not actually eliminating the duplicates yet but that should be easy once the "manual" shape construction actually works.

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 16

[Brian Hackett [Laid off!]]

Comment on attachment 594860 [details] [diff] [review]
Patch

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

::: js/src/jsscript.h
@@ +175,5 @@
>      HeapPtr<Shape> lastBinding;
>      uint16_t nargs;
>      uint16_t nvars;
>      uint16_t nupvars;
> +    uint16_t hasDup_;        // nonzero if there are duplicate argument names

This can be a bool hasDup_:1

Comment 17

[David Mandelin [:dmandelin]]

Good idea on the bitfield--thanks.

http://hg.mozilla.org/integration/mozilla-inbound/rev/2322fe48476e

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2322fe48476e

Comment 19

[Johnny Stenback (:jst)]

Dave, is this patch good for aurora? If so, please nominate there. What about esr?

Comment 20

[David Mandelin [:dmandelin]]

Comment on attachment 594860 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 700300
User impact if declined: leaves a potential security vulnerability in place
Testing completed (on m-c, etc.): running on m-c for 5 weeks
Risk to taking this patch (and alternatives if risky): Low. Mostly it affects processing only if there are duplicate argument names in a function, which is a rare (and not useful to the programmer) case. It's unlikely but possible I got something wrong in the refactoring of the normal cases.
String changes made by this patch: none

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 594860 [details] [diff] [review]
Patch

[Triage Comments]
Approved for Beta 12 and the ESR branch given the low risk evaluation of this sg:crit and where we are in the cycle.

Comment 22

[David Mandelin [:dmandelin]]

Landed to beta: 
http://hg.mozilla.org/releases/mozilla-beta/rev/c6c5fbe3eb36

Patch doesn't apply cleanly to ESR10. The bug is there, but the affected code was refactored significantly between then and now, so the first cut rebase just crashes all over the place in tests. Is it still worth landing to ESR10?

Comment 23

[Christian Holler (:decoder)]

Test committed with fix, marking verified by that.

Comment 24

[Alex Keybl [:akeybl]]

(In reply to David Mandelin from comment #22)
> Landed to beta: 
> http://hg.mozilla.org/releases/mozilla-beta/rev/c6c5fbe3eb36
> 
> Patch doesn't apply cleanly to ESR10. The bug is there, but the affected
> code was refactored significantly between then and now, so the first cut
> rebase just crashes all over the place in tests. Is it still worth landing
> to ESR10?

We're committed to taking all sg:crit bugs that affect FF10 on the ESR. We should move forward with a new fix.

Comment 25

[David Mandelin [:dmandelin]]

Attachment: Patch for ESR10

For some reason I didn't see akeybl's comment in my bugmail, but oddly enough on the same day I decided I needed to rebase this for a JS source release. With a manual rebase to ESR10, it seems to work fine. Brian, would you mind giving it a quick glance just to see if I made any mistakes? The only thing I really question is this line in jsscript.cpp:

   shape = shape->getChild(cx, *shapes[i], &shape);

Comment 26

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/dfaa5f24bd4a

Comment 27

[u279076]

Verified this does not assert using 2012-04-16 latest-mozilla-esr10 js-shell.

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

Ubuntu 11.10 32bit
Verified that the test from comment #0 doesn't assert on the latest mozilla-beta revision (8072115a9e89)

Marking verified for Firefox 13.

Comment 29

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug714614.js.