+++ This bug was initially created as a clone of Bug #1085746 +++ In bug 1085746 we fixed a very specific issue in |setFromTypedArray| [1] where we believe the b2g compiler (arm-linux-androideabi-gcc-4.7?) was optimizing an array copy in a way the caused an unaligned read in an instruction that required alignment. I'm pretty sure this is a regression that landed in 35, I haven't confirmed if it affects Fennec yet, but it definitely crashes FxOS 2.2 (just install a recent nightly). This is *extremely* easy to trigger, essentially: |new UIntArray(new IntArray(new ArrayBuffer(2048)).subarray(10, 1350));| My testing has shown this issue manifests in many of TypedArray combinations and needs to be fixed. I believe there are a few options: #1 - We can use the |volatile| trick again and hope it works #2 - We can try replacing the copy loops w/ std::copy or our own templated copy function Either way I'd like to measure the performance impact of this and see if we need to limit to just ARM platforms or if we can have a unified solution. [1] http://hg.mozilla.org/mozilla-central/annotate/76b85b90a8cb/js/src/vm/TypedArrayCommon.h#l166

Hmm. Did we really make the change in [1] *only* to the int8_t data path, and not to every possible target element type? That seems very incomplete to me, the way it is now.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1) > Hmm. Did we really make the change in [1] *only* to the int8_t data path, > and not to every possible target element type? That seems very incomplete > to me, the way it is now. Yep! Hence this bug.

Let's see if this builds, first, then if it fixes the j2me thing second: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46ed288331f5

Comment on attachment 8524769 [details] [diff] [review] Possible patch Review of attachment 8524769 [details] [diff] [review]: ----------------------------------------------------------------- I'll give this a test on my flame to see if it solves the crashes. My concern w/ just switching to std::copy (vs just volatile on ARM vs something else) is whether or not it has perf implications. I also get the feeling we'll run into compiler warnings due to signed/unsigned conversions (but the try run will let us know that).

|std::copy| seems to crash as well, in what looks like the UInt8->Int16 case, which feels subtly different. Some gdb output: Program received signal SIGBUS, Bus error. (gdb) bt #0 __copy<unsigned char*, short*, int> (__result=<optimized out>, __last=0xb33c9d50 "...", __first=<optimized out>) at ../../../../mozilla-central/build/stlport/stlport/stl/_algobase.h:214 #1 __copy_ptrs<unsigned char*, short*> (__result=0xb3cff000, __last=0xb33c9d50 "...", __first=0xb33c980a "...", <incomplete sequence \321>...) at ../../../../mozilla-central/build/stlport/stlport/stl/_algobase.h:262 #2 __copy_aux<unsigned char*, short*> (__result=0xb3cff000, __last=0xb33c9d50 "...", __first=0xb33c980a "...", <incomplete sequence \321>...) at ../../../../mozilla-central/build/stlport/stlport/stl/_algobase.h:277 #3 copy<unsigned char*, short*> ( __last=0xb33c9d50 "...", __result=0xb3cff000, __first=0xb33c980a "...", <incomplete sequence \321>...) at ../../../../mozilla-central/build/stlport/stlport/stl/_algobase.h:293 #4 setFromTypedArray (offset=3060445240, source=..., target=..., cx=<optimized out>) at ../../../../mozilla-central/js/src/vm/TypedArrayCommon. (gdb) frame 4 #4 setFromTypedArray (offset=3060445240, source=..., target=..., cx=<optimized out>) at ../../../../mozilla-central/js/src/vm/TypedArrayCommon.h:175 175 std::copy(src, src + count, dest); (gdb) l 170 break; 171 } 172 case Scalar::Uint8: 173 case Scalar::Uint8Clamped: { 174 uint8_t *src = static_cast<uint8_t*>(data); 175 std::copy(src, src + count, dest); 176 break; 177 }

Hm, right, signed-unsigned issues with std::copy here. Let's do this for now, then. (And next get this junk self-hosting ASAP so we don't have to have this nonsense.)

Comment on attachment 8524871 [details] [diff] [review] Just add volatile to all the places Review of attachment 8524871 [details] [diff] [review]: ----------------------------------------------------------------- No crashes for me and it's limited to ARM so I feel pretty comfortable w/ this. I'd like to know if there's a perf hit, but considering it was crashing before that's kind of hard to judge.

Comment on attachment 8524871 [details] [diff] [review] Just add volatile to all the places You don't get to worry about perf until you're not crashing memory-unsafely. :-)

Comment on attachment 8524871 [details] [diff] [review] Just add volatile to all the places [Security approval request comment] > How easily could an exploit be constructed based on the patch? Perhaps not at all. It's bogus compiler-generated code on ARM, and it might only manifest as unaligned accesses that'd immediately crash. Impossible to say without debugging the compiler. *touches nose* > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It's obvious there's a problem, comment notes it's an alignment issue. But there's no indication of worse consequences in the comment (because we don't know of any). > Which older supported branches are affected by this flaw? >If not all supported branches, which bug introduced the flaw? Bug 896116 landed in 35, so looks like aurora only. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Will backport straightforwardly. > How likely is this patch to cause regressions; how much testing does it need? Not likely to the former; <insert rant about doing any testing on ARM on tinderbox> but otherwise it's a very small change not really needing testing. Approval Request Comment [Feature/regressing bug #]: bug 896116 [User impact if declined]: unaligned accesses on ARM, possibly other shenanigans if the compiler goes really gonzo [Describe test coverage new/current, TBPL]: Well, IF WE WERE RUNNING TESTS ON ARM ON TINDERBOX (ahem), our existing tests would have to have caught this. But we're not, so. [Risks and why]: None, unless we're doing a sidestep away from one compiler bug, into another (given compilers basically give up optimizing when they see volatile, very unlikely). [String/UUID change made/needed]: N/A

Comment on attachment 8524871 [details] [diff] [review] Just add volatile to all the places Approvals given. Let's get this on trunk and Aurora and call it good. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c3ea2e8d43 https://hg.mozilla.org/releases/mozilla-aurora/rev/476779d0d240