986864 - Crash [@ memmove] or [@ mozilla::PodCopy] or [@ js_NewStringCopyN] or Assertion failure: PointerRangeSize(src, static_cast<const T*>(dst)) >= nelem, at dist/include/mozilla/PodOperations.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: debug and opt stacks

function x() {}
for (var j = 0; j < 99999; ++j) {
    (function() {
        x += x.watch("endsWith", SharedArrayBuffer);
        return 0 >> Function(x)
    })()
}

crashes js debug shell on m-c changeset 573ef29c7a9f with --ion-eager --ion-parallel-compile=off at memmove with mozilla::PodCopy and js_NewStringCopyN on the stack. It also crashes js opt shell at memmove with js_NewStringCopyN on the stack.

Sometimes this assertion pops up: Assertion failure: PointerRangeSize(src, static_cast<const T*>(dst)) >= nelem, at dist/include/mozilla/PodOperations.h

s-s because such varying signatures are scary.

My debug 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 --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

My opt 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 --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140226135810" and the hash "ac9709133b27".
The "bad" changeset has the timestamp "20140226135912" and the hash "7bb98e166b5b".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ac9709133b27&tochange=7bb98e166b5b

Sean, only bug 933001 is on the stack, is it a likely regressor?

Comment 1

[Sean Stangl [:sstangl]]

This is not a SharedArrayBuffer bug -- any fuzz result that uses "SharedArrayBuffer" as a constructor will bisect at least to the patch that introduces it. In this case, the following modified testcase also reproduces the crash (on x64, with --no-ion --no-baseline):

function x() {}
for (var j = 0; j < 99999; ++j) {
  (function() {
    x += x.watch("endsWith", ArrayBuffer);
    return 0 >> Function(x);
  })()
}

I would recommend bisecting with this new version.

Comment 2

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

Thanks Sean!

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/78a029898775
user:        Jon Coppeard
date:        Fri Feb 07 11:20:38 2014 +0000
summary:     Bug 961091 - Perform GC if necessary on exit from engine and on interpreter allocation r=terrence

Jon, seems like bug 961091 might be a more likely regressor, would it?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug986864-autoDeferPurge

The problem is with SourceDataCache::AutoSuppressPurge.  SourceDataCache maps from ScriptSource* to decompressed source chars.  AutoSuppressPurge stops this cache being cleared on GC, but if a ScriptSource* that has an entry in the table is destroyed by the collection then its entry in the cache will not be removed.

Then, if a different ScriptSource is created at the same address, the cache will return the data for the old entry.

ScriptSource::substring() assumes the length of the return string, and in this case it's not what it expects and it tries to copy far more data out of it than it actually contains, which results in the assertion failures seen here.

The solution is not to suppress the purge, but merely defer it while someone can be accessing the data in the cache.  I changed AutoSuppressPurge to AutoDeferPurge, which will perform the purge in its destructor if a GC happens while it's on the stack.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8397143 [details] [diff] [review]
bug986864-autoDeferPurge

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

As discussed IRL, it would be simpler to not use any RAII protection at all, and instead allocate a string of the right length in advance, then copy the character data in when you can't GC anymore.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug986864-scriptSource2

Unfortunately it turned out that we couldn't ensure there was no GC whenever these chars where in use.

Here's new patch.  This adds a a holder object that takes ownership of the chars for a specific ScriptSource if the cache is purged, and deletes them itself when it goes out of scope.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8398484 [details] [diff] [review]
bug986864-scriptSource2

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

Had to stare at it for a while, but it looks good to me now.

::: js/src/jit-test/tests/gc/bug-986864.js
@@ +1,5 @@
> +// |jit-test| slow
> +function x() {}
> +for (var j = 0; j < 9999; ++j) {
> +    (function() {
> +        x += x.watch("endsWith", SharedArrayBuffer);

It would be much better if we could trigger this with something other than SharedArrayBuffer. That will get automatically disabled on Aurora and everything after, breaking this test.

::: js/src/jsscript.cpp
@@ +1423,5 @@
> +    // The holder is going out of scope.  If it has taken ownership of cached
> +    // chars then delete them, otherwise unregister ourself with the cache.
> +    if (charsToDelete_) {
> +        JS_ASSERT(!cache_ && !source_);
> +        js_delete(const_cast<jschar *>(charsToDelete_));

I think charsToDelete_ was allocated with js_realloc, so this should really be js_free instead of js_delete. (Maybe that means it should be charsToFree_?)

Comment 7

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

(In reply to Steve Fink [:sfink] from comment #6)
> ::: js/src/jit-test/tests/gc/bug-986864.js
> @@ +1,5 @@
> > +// |jit-test| slow
> > +function x() {}
> > +for (var j = 0; j < 9999; ++j) {
> > +    (function() {
> > +        x += x.watch("endsWith", SharedArrayBuffer);
> 
> It would be much better if we could trigger this with something other than
> SharedArrayBuffer. That will get automatically disabled on Aurora and
> everything after, breaking this test.

Let's use the test Sean made in comment 1 - it doesn't use SharedArrayBuffer, instead only relying on ArrayBuffer.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug986864-scriptSource-v3

Updated patch with review comments addressed.

Comment 9

[Jon Coppeard (:jonco)]

AutoSuppressPurge was introduce in bug 938615.

Comment 10

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

(In reply to Jon Coppeard (:jonco) from comment #9)
> AutoSuppressPurge was introduce in bug 938615.

Will this be the correct regressing bug? If so, this issue goes back to version 28.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

So the problem is that this cache can contain a stale entry for a dead ScriptSource* and then hand out the pointer to its (live) source string.  The callers here assume the length of the string, but they only read from it.  So they may read source data for the wrong thing, which may be controllable by an attacker, and they may also read past the end of it.

An attack might be possible to get code in a privileged compartment to execute javascript code controllable by an attacker, but it seems this would be hard to arrange.  It would involve getting GC to trigger at a very specific point, and then somehow getting the privileged compartment to allocate a ScriptSource at the same address as a dead ScriptSource set up by the attacker.

So I think the answer is that it would be possible but difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I would say it's not obvious from the patch.

Which older supported branches are affected by this flaw?

26 onwards.

If not all supported branches, which bug introduced the flaw?

Bug 938615.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

It should be possible to apply the same patch.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.  Letting it bake on central for a few days should be fine.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

sec-approval+ for trunk.

If this is 26 onwards, why doesn't it affect b2g 1.2, which is derived from 26?

Comment 13

[Jon Coppeard (:jonco)]

Sorry, this was introduced in 28 (not 26).

Comment 14

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72ba89a774db

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72ba89a774db

Please request Aurora/Beta approval on this ASAP.

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 938615
User impact if declined: Potential security issue.
Testing completed (on m-c, etc.): On m-c since 2014-03-31.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8bdf848dd9b
https://hg.mozilla.org/releases/mozilla-beta/rev/e6b88dfe88cd
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e8649867ab24

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.