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

VERIFIED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: dmandelin)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla13
x86
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox9 wontfix, firefox10- wontfix, firefox11- wontfix, firefox12+ verified, firefox13+ verified, firefox-esr1012+ verified)

Details

(Whiteboard: [sg:critical][qa!])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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()();
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.
Group: core-security
(Assignee)

Updated

6 years ago
Whiteboard: js-triage-needed → [sg:critical]
Dave, can you assign this appropriately?
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox9: --- → wontfix
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox12: --- → +
tracking-firefox9: --- → -
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.
Blocks: 700300
status-firefox10: affected → ---
status-firefox11: affected → ---
status-firefox12: affected → ---
status-firefox9: wontfix → ---
tracking-firefox10: + → ---
tracking-firefox11: + → ---
tracking-firefox12: + → ---
tracking-firefox9: - → ---
(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.

Updated

5 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox9: --- → wontfix
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox12: --- → +

Updated

5 years ago
No longer blocks: 700300
(Assignee)

Comment 5

5 years ago
This won't be fixed in time for 10.
status-firefox10: affected → wontfix
tracking-firefox10: + → -
Assignee: general → dmandelin
(Assignee)

Comment 6

5 years ago
Created attachment 592306 [details] [diff] [review]
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).
Attachment #592306 - Flags: review?(luke)

Comment 7

5 years ago
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.
Attachment #592306 - Flags: review?(luke) → review+
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/602d30678cac
Target Milestone: --- → mozilla12
(Assignee)

Comment 9

5 years ago
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.
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.
(Assignee)

Comment 11

5 years ago
(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...
(Assignee)

Comment 12

5 years ago
(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.
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 592963 [details] [diff] [review]
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.
Attachment #592306 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 594860 [details] [diff] [review]
Patch
Attachment #592963 - Attachment is obsolete: true
Attachment #594860 - Flags: review?(bhackett1024)
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
Attachment #594860 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 17

5 years ago
Good idea on the bitfield--thanks.

http://hg.mozilla.org/integration/mozilla-inbound/rev/2322fe48476e
https://hg.mozilla.org/mozilla-central/rev/2322fe48476e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
Dave, is this patch good for aurora? If so, please nominate there. What about esr?
status-firefox11: affected → wontfix
status-firefox13: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox11: + → -
tracking-firefox13: --- → +

Updated

5 years ago
tracking-firefox-esr10: ? → 13+
(Assignee)

Comment 20

5 years ago
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
Attachment #594860 - Flags: approval-mozilla-esr10?
Attachment #594860 - Flags: approval-mozilla-beta?

Updated

5 years ago
tracking-firefox-esr10: 13+ → 12+
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.
Attachment #594860 - Flags: approval-mozilla-esr10?
Attachment #594860 - Flags: approval-mozilla-esr10+
Attachment #594860 - Flags: approval-mozilla-beta?
Attachment #594860 - Flags: approval-mozilla-beta+
status-firefox-esr10: --- → affected
(Assignee)

Comment 22

5 years ago
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?
(Reporter)

Comment 23

5 years ago
Test committed with fix, marking verified by that.
Status: RESOLVED → VERIFIED
(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.
status-firefox12: affected → fixed
(Assignee)

Comment 25

5 years ago
Created attachment 612058 [details] [diff] [review]
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);
Attachment #612058 - Flags: feedback?(bhackett1024)
Attachment #612058 - Flags: feedback?(bhackett1024) → feedback+
(Assignee)

Comment 26

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/dfaa5f24bd4a
status-firefox-esr10: affected → fixed
Whiteboard: [sg:critical] → [sg:critical][qa+]
Verified this does not assert using 2012-04-16 latest-mozilla-esr10 js-shell.
status-firefox-esr10: fixed → verified

Updated

5 years ago
status-firefox12: fixed → verified
Group: core-security
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.
status-firefox13: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
(Reporter)

Comment 29

4 years ago
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug714614.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.