The interpreter engine for the core JavaScript language, independent of the browser's object model. File ONLY core JavaScript language bugs in this category. For bugs involving browser objects such as "window" and "document", use the "DOM" component. For bugs involving calls between JavaScript and C++, use the "XPConnect" component.

In most places the patch just reorders IndexToId and OBJ_GET_PROPERTY calls so a getter would not have a chance to kill the atom behind a large index.
The exception to this rule is in array_reverse and array_pop methods where the ids when necessary are protected with JS_KEEP_ATOMS call and array_extra where for the MAP case the id is re-build again to simplify the implementation.
The patch is not minimal since I could not to resist and optimized IndexToId to avoid constructing temporary JSString.

Initially I thought that a generic way to fix the problem would be to make sure that GC call from the branch callback would always keep atoms. But it is way to easy to prevent collection of atoms at all. So with the patch from comment 1 I took a conservative approach of rooting the atoms in jsarray.c. This is the reason for changing the bug title as the focus here is on just jsarray.c.

(In reply to comment #5)
> I guess it is way to late for 1.8.0.5 approval but I ask in any case. And if
> this is too late indeed, then I think I have to make comments in the patch less
> explicit about making an exploit.
Or wait with a trunk commit until 1.8.0.6

As far as I can see jsarray.c is the only source of this type of hazards in the engine. Other users of js_Atomize* either root the atoms via JS_KEEP_ATOMS or they never call getter/setter or other scriptable code (js_XDRAtom case).

Comment on attachment 227465[details][diff][review]
Fix
Drive-by, thanks for fixing:
+ * a branch callback call and the latter invokes js_GC without
+ * GC_KEEP_ATOMS flag, GC would collect the atom that we would access later
"the" after "without"
+ if (len > JSVAL_INT_MAX + 1)
To my eyes len >= JSVAL_INT_MAX + 1 is slightly clearer.
+ ok = ok && IndexToId(cx, len - i - 1, &id2) &&
+ PropertyExists(cx, obj, id2, &id2exists) &&
+ (!id2exists || OBJ_GET_PROPERTY(cx, obj, id2, tmproot2));
You clearly want JS2's &&= operator ;-).
Breaking as soon as ok is false would use the same number of instructions but be faster in a failure case. That doesn't matter, but it is the way most code in SpiderMonkey is written. And retesting ok in "ok = ok && ..." after previous successes is unnecessary in the case we want to optimize (no failures). So I'm still in favor of breaking on first failure.
This repeated comment:
+ * Construct id2 after we got property's value not to worry
+ * about getter there triggering GC and collectiong id2.
could be reworded to say "to not worry" -- splitting an infinitive is ok in such cases. The longer way around is "so as not to worry".
/be

(In reply to comment #9)
> And retesting ok in "ok = ok && ..." after previous
> successes is unnecessary in the case we want to optimize (no failures).
I claim a sane compiler would do exactly the same number of tests in
ok = A && B
and
ok = A
ok = ok && B
In the first case the compiler must test for A to avoid calculating B. Similarly in the second case the compiler has to test ok to avoid calculating B. But there is no need to test the results of A in ok = A.
In the initial version of the patch I just replaced "return JS_FALSE" by "goto bad". When I discover later that I missed one replacement (probably because it scrolled beyond the top edge of the window), I gave up and rewrote it as in the patch.

(In reply to comment #10)
> (In reply to comment #9)
> > And retesting ok in "ok = ok && ..." after previous
> > successes is unnecessary in the case we want to optimize (no failures).
>
> I claim a sane compiler would do exactly the same number of tests in
> ok = A && B
> and
> ok = A
> ok = ok && B
I know, that was not my point (I adverted to the equivalence of these forms when I wrote "You clearly want JS2's &&= operator ;-)").
My point was that most SpiderMonkey code breaks on failure to avoid retesting ok on success. Break meaning 'break', 'goto out', 'goto bad', etc.
> In the initial version of the patch I just replaced "return JS_FALSE" by "goto
> bad". When I discover later that I missed one replacement (probably because it
> scrolled beyond the top edge of the window), I gave up and rewrote it as in the
> patch.
Ok. I have a taller window ;-).
/be

Comment on attachment 227465[details][diff][review]
Fix
>--- .pc/array_atom_roots.diff/js/src/jsarray.c 2006-06-26
>+ * Construct id2 after we got property's value not to worry
>+ * about getter there triggering GC and collectiong id2.
I think I'd prefer s/got/after we've gotten/ in this comment, and please fix the 'collecting' typo, here and where you've copy/pasted the comment. Other than that, this looks good.

Changes compared with version 1:
1. I reverted to the "break early" style in array_reverse and array_pop to preserve code style.
2. Comments are shortened but I compensated that by references to the bug.

Comment on attachment 227749[details][diff][review]
Minimal fix
+ * Find id2 after OBJ_GET_PROPERTY call as it can invalidate
+ * ids for large indexes. See bug 341956.
It would be nice to compress this to a one-line comment. How about
/* Get id after value to avoid nested GC hazards. */
or something like that?
sr=me anyway.
/be

I asked on irc.mozilla.org #developers, since that channel is "sheriff". preed said "I think 02 just isn't set to build yet or something. It's taking half of the UB builds that 01 does." The red balsa seamonkey-ports was due to a gcc bug.
The tree remains open, so I think you should check in.
/be

Comment on attachment 228293[details]
js1_5/GC/regress-341956.js
Note that the test can check that good === true for booletproof checking. Plus it just covers unshift case, but what about the examples for reverse and pop?

Tested with the original test version.
verified fixed 1.8.0.5, 1.9a1 windows/macppc/linux 20060706 builds.
Not Fixed in 1.8.1 20060706. Crashes shell windows, browser windows/macppc/linux.
confirmed that windows browser crashes on all of the new tests 01, 02, and 03 in opt but not debug builds and windows shell crashes on all the new tests for opt and debug.