try { Object.defineProperty(this, "x", { get: function() { return y.length } }) z = ArrayBuffer(4) y = Int8Array(z) z(x) } catch (e) {} for (v in x) {} neuter(z) print(x) $ ./js-opt-64-dm-ts-darwin-ebdf2740dc3e --fuzzing-safe --ion-parallel-compile=off test.js 0 $ ./js-opt-64-dm-ts-darwin-ebdf2740dc3e --fuzzing-safe --ion-parallel-compile=off --ion-eager test.js 4 My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/6d4ff510c117 user: Luke Wagner date: Thu Oct 24 08:59:59 2013 -0500 summary: Bug 929786 - Add shell function to neutering (r=sfink) (s-s because this involves ArrayBuffers) Since this seems related to ArrayBuffers and neutering, I'm cc'ing the gurus here, Waldo and sfink. (Bug 929786 might just have exposed the issue) Waldo, is this related?

Cleaner testcase: var ab = new ArrayBuffer(4); var ta = new Int8Array(ab); function q() { return ta.length; } q() + q(); neuter(ab); assertEq(q(), 0); We hit IonBuilder::getTypedArrayLength and inline the typed array's length. All fine, because when we do that we |obj->setImplicitlyUsedUnchecked();|. Or is it? During the neutering operation we call MarkObjectStateChange on the typed array, which *should* throw away the JIT code that inlines the old length. But if I step into that call a bit, it's clearly not a constraint associated with |obj->setImplicitlyUsedUnchecked()| -- just a TypeCompilerConstraint<ConstraintDataFreezeObjectFlags> watching for the single flag OBJECT_FLAG_UNKNOWN_PROPERTIES (presumably not generated by this code at all). As far as I can tell, there's no constraint added here that'll cause the inlined typed array length constant to ever be thrown away if the underlying ArrayBuffer is neutered. And, going back through blame, it looks like "implicitly used" is only relevant for purposes of preserving against DCE seemingly "dead" code to compute the object. It has *nothing* to do with a constraint watching for typed array/arraybuffer changes at all. So, this is pretty much bug 982974 comment 33 and bug 982974 comment 43 (responding to that comment). Taking. I'll post a fix Monday.

The existing watchStateChangeForTypedArrayBuffer is inadequate because of bug 999651. And really, in this case, we don't really care about the data pointer changing anyway. Rather, we should just have this case guard on only the length. (The buffer case perhaps should depend on both, because implicitly it's a constraint that depends on Range(data pointer, length) always being valid. But that's for bug 999651.)

Jeff, what security rating should we give this? sec-high? Just a leak?

The inlined-constant length can get burned into bounds checks in IonBuilder::jsop_getelem_typed and IonBuilder::jsop_setelem_typed. If you're careful, it looks like things will fall over accordingly. That makes this critical.

Well, actually. This only gets used for bounds-checks in conjunction with getTypedArrayElements, which does have a guard in it that detects *some* neutering, per bug 999651. The object has to be constant for length inlining. But the data case has an *additional* restriction on when a not-neutered constraint is added -- the typed array data can't be in the nursery. So *if* the typed array's data is in the nursery, we'll inline a bad length, add no constraint, then load the data pointer out of the object. Because of the back-neutered-arrays-with-ballast trick from bug 982974, this shouldn't index out of bounds of actual memory (at least, not once bug 999140 lands, and ballasts are used everywhere). So maybe we are safe here against anything but incorrect results. Still, I wouldn't want to take that bet unless I absolutely had to.

How far back does this go?

Comment on attachment 8410541 [details] [diff] [review] Test Review of attachment 8410541 [details] [diff] [review]: ----------------------------------------------------------------- I know it's something else, but could you also test a larger (always out-of-line) ArrayBuffer?

(In reply to Al Billings [:abillings] from comment #7) > How far back does this go? Given comment 6, it sounds like it requires GGC (which introduced the nursery), so just to 31? Waldo?

Comment on attachment 8410544 [details] [diff] [review] Patch Review of attachment 8410544 [details] [diff] [review]: ----------------------------------------------------------------- I hate to pile things on someone else, but I'm too unfamiliar with the constraint system to feel confident in my ability to review this.

Comment on attachment 8410544 [details] [diff] [review] Patch Review of attachment 8410544 [details] [diff] [review]: ----------------------------------------------------------------- I think we should just use the new ConstraintDataFreezeObjectForTypedArrayData as patched in bug 999651 instead of adding a new constraint just to check the length. I highly doubt there are real perf gains to separate the two -- if the programmer neutered the array, I see no reason for us to take extra pains to stay in the JIT.

The bad-length visible behavior dates back "a ways". The wrong code is in mozilla-beta, mozilla-release, and looks to be in b2g18 as well. As for when bad-indexing happens, less clear. The GGC aspect of the data-access code doesn't date back so far. Prior to the GGC conditioning, it looks like bad lengths were inlined unconditionally for any deemed-constant object. So, GGC doesn't have much of anything to do with this issue, at least sometimes. The safe inference/assumption is that this needs to be fixed everywhere, I think, with GGC being largely irrelevant to the analysis.

I'm abandoning the patch (but not the test) here in favor of fixing this as part of bug 999651's ultimate patch.

Fixed in a mega-rollup patch that also contains bug 999651's fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c631967ab9e Test not to land yet, probably in a cycle or two.

Backed out, then relanded with CLOBBER: https://hg.mozilla.org/integration/mozilla-inbound/rev/57b0932e2f06

Based on comment 9, marking 30 and ESR24 as unaffected.

(In reply to Lawrence Mandel [:lmandel] from comment #17) > Based on comment 9, marking 30 and ESR24 as unaffected. Not true, see: (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12) > The safe inference/assumption is that this needs to be fixed everywhere, I > think, with GGC being largely irrelevant to the analysis.

Wow. Missed the end of comment 12. Good catch and thanks Gary. Given that this is sec-critical, we need to consider branch patches. Given where we are in the cycle, the sooner the better but do take the time needed on central as the fix only landed today.

This is being patched and fixed in bug 999651's work -- fixed on trunk, fixed aurora/beta as of a few minutes ago, and further backports are almost ready to go for approval.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/57cd741e4d0b https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4c359ff76654

Hi Gary, any chance you can verify fixed on Fx30 and/or Fx24.6.0esr?

6d4ff510c117 is needed to expose the issue, and this changeset is not present on ESR 24. neuter now accepts a different number of arguments on mozilla-beta (30), mozilla-aurora (31) and on mozilla-central (32), so the testcases no longer work the way they do. (e.g. now I get "Error: wrong number of arguments to neuter()" on all branches) Waldo, any idea how the testcases should be changed? Else, we should just consider this fixed and move on.

Here's an updated test that deals with bug 999755's addition to all branches. As I recall, this might have been one of the tests that on the very earliest branches (esr24, maybe b2g26 but I don't think so) I couldn't make fail pre-patch and pass post-patch. (I suspect because of object-constantizing constraints I didn't have enough knowledge to work through to write a working testcase. At least, not without spending too much time doing so.) So this may be unverifiable even with this slightly-updated test.

Thanks Waldo. I'll retry verification again soon. :)

Using Waldo's new tests in comment 26, I was able to verify a "before (assert) / after (no assert)" situation for aurora (32) due to the original changeset listed in comment 0, rev ebdf2740dc3e, being present. I was only able to verify "after (no assert)" situations for beta (31) and release (30) due to that changeset not existing on the branches. Nonetheless, let's assume verified.

trying to apply this to SeaMonkey 2.26.1 (Gecko 29) resulted in patch conflicts, and due to the nature of this patchset it seems like I won't be able to take it.