1263857 - Crash [@ js::gc::MaybeForwarded] or Assertion failure: (ptrBits & 0x7) == 0, at dist/include/js/Value.h:854

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 29d5a4175c8b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

See attachment.

Backtrace:

0   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100022271 JS::Value::toObject() const + 257 (Value.h:854)
1   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100a335fe decltype(fp(static_cast<JSObject*>(std::nullptr_t)mozilla::Forward<JS::CallbackTracer*&, char const*&>(fp1))) js::DispatchTyped<DoCallbackFunctor<JS::Value>, JS::CallbackTracer*&, char const*&>(DoCallbackFunctor<JS::Value>, JS::Value const&, JS::CallbackTracer*&&&, char const*&&&) + 158 (Value.h:1889)
2   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100a30014 JS::Value DoCallback<JS::Value>(JS::CallbackTracer*, JS::Value*, char const*) + 36 (Tracer.cpp:70)
3   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x000000010062cc9c JSObject::traceChildren(JSTracer*) + 204 (TracingAPI.h:247)
4   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100a30188 js::TraceChildren(JSTracer*, void*, JS::TraceKind) + 40 (Tracer.cpp:127)
5   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005e731f js::gc::GCRuntime::checkForCompartmentMismatches() + 431 (jsgc.cpp:3948)
6   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005e75b3 js::gc::GCRuntime::beginMarkPhase(JS::gcreason::Reason) + 51 (jsgc.cpp:3985)
7   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005f3513 js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) + 771 (jsgc.cpp:6072)
8   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005f443c js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) + 460 (jsgc.cpp:6363)
9   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005f4da5 js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) + 741 (jsgc.cpp:6465)
10  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005e41d6 js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) + 86 (jsgc.cpp:6526)
11  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100a0bd94 JSString* js::gc::GCRuntime::tryNewTenuredThing<JSString, (js::AllowGC)1>(js::ExclusiveContext*, js::gc::AllocKind, unsigned long) + 260 (GCRuntime.h:673)
12  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008a9669 JSDependentString::new_(js::ExclusiveContext*, JSLinearString*, unsigned long, unsigned long) + 345 (String-inl.h:191)
13  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008a1f60 js::NewDependentString(JSContext*, JSString*, unsigned long, unsigned long) + 672 (String.cpp:1046)
14  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x000000010067188c js::SubstringKernel(JSContext*, JS::Handle<JSString*>, int, int) + 444 (jsstr.cpp:603)

Setting s-s because the regression window seems to point to something RegExp-related again.

Comment 1

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

Attachment: Testcase

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c5e0ea1a1ed2
user:        Tooru Fujisawa
date:        Sat Sep 05 22:01:41 2015 +0900
summary:     Bug 887016 - Part 11: Implement RegExp.prototype[@@replace] and call it from String.prototype.replace. r=h4writer,till

arai-san, is bug 887016 a likely regressor?

Comment 3

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

The testcase in comment 1 was reduced from a crash at js::gc::MaybeForwarded.

Comment 4

[Tooru Fujisawa [:arai]]

For now, the testcase gets reduced to following, I got same assertion failure |MOZ_ASSERT((ptrBits & 0x7) == 0);|

gcparam("maxBytes", gcparam("gcBytes") + 1);
fullcompartmentchecks(true);
/x/g[Symbol.replace](" x".repeat(32768), "");

so, this should be a regression from bug 887016, will continue investigating.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

This might be something related to bug 1240353 or perhaps more old thing.

In the fallback path for createGCObject in JitCompartment::generateRegExpMatcherStub, we used gc::Allocate and it doesn't initialize slots (set to 0x42 in debug mode) for match result object.

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/js/src/jit/CodeGenerator.cpp#1562
>     masm.createGCObject(object, temp2, templateObject, gc::DefaultHeap, &matchResultFallback);
>     masm.bind(&matchResultJoin);
> ...
>     masm.bind(&matchResultFallback);
>     CreateMatchResultFallback(masm, regsToSave, object, temp2, temp5, templateObject, &oolEntry);
>     masm.jump(&matchResultJoin);

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/js/src/jit/CodeGenerator.cpp#1462
> CreateMatchResultFallbackFunc(JSContext* cx, gc::AllocKind kind, size_t nDynamicSlots)
> {
>     return js::Allocate<JSObject, NoGC>(cx, kind, nDynamicSlots, gc::DefaultHeap,
>                                         &ArrayObject::class_);

Then, the match result object's slots are not initialized until the last step in RegExpMatcherStub:

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/js/src/jit/CodeGenerator.cpp#1684
>     masm.storeValue(JSVAL_TYPE_INT32, sticky, Address(temp2, 0));
>     masm.storeValue(JSVAL_TYPE_STRING, input, Address(temp2, sizeof(Value)));

Before reaching there, we could jump to OOL path while constructing match strings.

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/js/src/jit/CodeGenerator.cpp#1633
>         Label* failure = &oolEntry;
> ...
>             depStr[isLatin].generate(masm, cx->names(), isLatin, temp3, input, temp4, temp5,
>                                      stringIndexAddress, stringLimitAddress, failure);

If it jumps to OOL path, the match result object created in JIT gets left uninitialized, and the slots are traversed in GC.

I think this is same for when fallback path is not used (createGCObject succeeds), but I don't know how to hit it.

Anyway, we should initialize the slots of the match result object as soon as we create the object, at least before jumping to OOL path.

> +     masm.loadPtr(Address(object, NativeObject::offsetOfSlots()), temp2);
> +     masm.storeValue(UndefinedValue(), Address(temp2, 0));
> +     masm.storeValue(UndefinedValue(), Address(temp2, sizeof(Value)));

Those temporary values won't be used in successful case, so I used undefined, is it okay?

Comment 6

[Tooru Fujisawa [:arai]]

When createGCObject succeeds, the value of slots are 0xfffc2f2f2f2f2f2f,
and when createGCObject fails and fallbacks to js::Allocate, the value of slots are 0xfffc000000000042.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8740390 [details] [diff] [review]
Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

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

I have two concerns here:
1) In initGCThing we should probably support when there are dynamic slots in ArrayObject. This is quite uncommon therefore this case is probably not implemented.
2) We should init to the default values of the templateObject, not UndefinedValue. (Because types need to be correct!)

I'm fine with adjusting this code to use templateObject slot values for now. And doing the more general case (1) in a follow-up bug.
Given that it might be less code and easier to backport if needed and less prone to introduce new bugs.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Thank you for your feedback :)
Implemented (2).

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8740698 [details] [diff] [review]
Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

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

Thanks! Good catch!

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8740698 [details] [diff] [review]
Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
The testcase in this patch uses shell-only function to change the GC behavior, so it's not directly usable for exploit code,
but it might be able to create based on this.


> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, the comment there describes that the slot value (can be a pointer) was not initialized.


> Which older supported branches are affected by this flaw?
The particular case handled in this bug is affected only in nightly (from bug 887016),
but the basic structure of the code there was not much changed from Firefox 35, and all supported branched have it.
Here's the bugs that touched the code:
  * bug 887016  - Firefox 48
  * bug 1240353 - Firefox 47
  * bug 1207922 - Firefox 46
  * bug 1066828 - Firefox 35

I have no idea how to hit the issue on older branch tho, all branches could be affected.


> If not all supported branches, which bug introduced the flaw?
Exposed by bug 887016, but the underlying issue was there in all branches.


> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It should be easily prepared, as there is not so much difference across branches.


> How likely is this patch to cause regressions; how much testing does it need?
Not likely, just initializes temporary-uninitialized slot.
Automated test should catch if anything happen.

Comment 11

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

Comment on attachment 8740698 [details] [diff] [review]
Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

sec-approval+ for trunk.

We'll want this on Aurora but it is too late for beta or ESR45 (or ESR38).

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Here's the patch for mozilla-aurora.
The testcase in original patch doesn't work on mozilla-aurora, so no testcase attached.

will ask approval after landing to m-c.

Comment 13

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c77b965d8c74749ddc17f3c5744c950c64df3a3a
Bug 1263857 - Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub. r=h4writer

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c77b965d8c74

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8741229 [details] [diff] [review]
(mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Approval Request Comment
> [Feature/regressing bug #]
* whole possibly-affected code path was added by bug 1066828 (Firefox 35)
* the code that is confirmed to crash was added by bug 1240353 (Firefox 47)
* exposed by bug 887016 (Firefox 48)

> [User impact if declined]
Reading random address while GC, that will cause crash, or perhaps some exploit (not sure how to exploit it tho).

> [Describe test coverage new/current, TreeHerder]
Tested in m-c TreeHerder.

> [Risks and why]
Very low, just initializes temporary-uninitialized slot.

> [String/UUID change made/needed]
None

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8741229 [details] [diff] [review]
(mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

canceling approval due to possible regression (timeout) on Windows xp debug (bug 1265187)

before backing out bug 1263857 (hits timeout)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=72db514353fe
backout bug 1263857 (green)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=76509843d14b
backout bug 1263857's testcase (green)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=08c11cbef4d8

So, the testcase seems to hit another issue there.
(The patch for mozilla-aurora doesn't contain the testcase tho)

Comment 18

[Tooru Fujisawa [:arai]]

In failure case, "--no-baseline --no-ion" variant for bug1263857.js is not logged there.
That shouldn't be affected by the actual code change in this patch.

Comment 19

[Tooru Fujisawa [:arai]]

locally, running the test directly shows following:

> E:\jsshell>js.exe --no-baseline --no-ion ../a.js
> Assertion failure: [unhandlable oom] OOM in createJitRuntime, at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jscntxt.cpp:1235
> Hit MOZ_CRASH() at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jscntxt.cpp:1236

Isn't it catch-able with "// |jit-test| allow-oom; allow-unhandlable-oom" ?

(Now preparing testing environment on VM, it may take some more time)

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Crash dialog shown by bug1263857.js

This crash dialog is shown when I run following command (it's in japanese tho...)
  $ ./jit_test.py ../../../js.exe --no-slow --no-progress --format=automation --jitflags=all bug1263857

Here's the log:

  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code 3, args "")
  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code 3, args "--ion-eager --ion-offthread-compile=off")
  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code 3, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code 3, args "--baseline-eager")
  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code 3, args "--baseline-eager --no-fpu")

It stops until I hit the "Don't Send" button in the crash dialog, and when I hit the button, the test continues, and it passes all tests:

  TEST-PASS | fx\jit-test\jit-test\tests\auto-regress\bug1263857.js | Success (code -2147483645, args "--no-baseline --no-ion")
  PASSED ALL
  Result summary:
  Passed: 6
  Failed: 0

Is this known issue?
How can I avoid this with unhandlable-oom testcase?

Comment 21

[Tooru Fujisawa [:arai]]

Disabled the testcase (in bug 1265187)
  https://hg.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08

Comment 22

[Tooru Fujisawa [:arai]]

the return value of strncmp is used wrongly :|

https://dxr.mozilla.org/mozilla-central/rev/1da1937a9e03154ae7c60089f2dcf5ad9ee20fa3/js/src/shell/js.cpp#7120
> if (crash_option && strncmp(crash_option, "1", 1)) {

it needs "!"

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: (followup) Disable windows crash reporter on automated tests.

Flipped the condition.

Comment 24

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

Comment on attachment 8742314 [details] [diff] [review]
(followup) Disable windows crash reporter on automated tests.

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

Wow. In scanning through the source code, we use a lot of different ways to test environment variables. After const char* v = getenv("SOMETHING"), I see:

  v
  v && *v
  v && v[0] != '0'
  v && strlen(v)
  v && !strncmp(v, "1", 1)
  v && sscanf(v, "%u", &val) == 1
  v && ParseInt(v)
  ...and some other case-specific ones

Given that this one is tricky to get right, I think I'd rather use one of the others. I believe this would maintain the same semantics (not that that's all that important):

  if (crash_option && crash_option[0] == '1')

r=me with the above, ask for re-review if you pick something else. (Either is fine with me.)

Comment 25

[Tooru Fujisawa [:arai]]

Comment on attachment 8741229 [details] [diff] [review]
(mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Now that the test issue on Windows xp was not related to this patch itself, so asking aurora approval (I mean mozilla47, maybe beta yet?) again.


Approval Request Comment
> [Feature/regressing bug #]
* whole possibly-affected code path was added by bug 1066828 (Firefox 35)
* the code that is confirmed to crash was added by bug 1240353 (Firefox 47)
* exposed by bug 887016 (Firefox 48)

> [User impact if declined]
Reading random address while GC, that will cause crash, or perhaps some exploit (not sure how to exploit it tho).

> [Describe test coverage new/current, TreeHerder]
Tested in m-c TreeHerder.

> [Risks and why]
Very low, just initializes temporary-uninitialized slot.

> [String/UUID change made/needed]
None

Comment 26

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e9e16e010ff25f3fd1ff2c2911dee4d1b8b0d2
Bug 1263857 - (followup) Disable windows crash reporter on automated tests. r=sfink

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Re-enable bug 1263857 testcase.

Re-enabled the testcase (this is actually for bug 1265187, but posting here as this is still s-s)

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6e9e16e010f

Comment 29

[Tooru Fujisawa [:arai]]

Comment on attachment 8741229 [details] [diff] [review]
(mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Looks like this missed the aurora merge. changing to beta approval for mozilla47


Approval Request Comment
> [Feature/regressing bug #]
* whole possibly-affected code path was added by bug 1066828 (Firefox 35)
* the code that is confirmed to crash was added by bug 1240353 (Firefox 47)
* exposed by bug 887016 (Firefox 48)

> [User impact if declined]
Reading random address while GC, that will cause crash, or perhaps some exploit (not sure how to exploit it tho).

> [Describe test coverage new/current, TreeHerder]
Tested in m-c TreeHerder.

> [Risks and why]
Very low, just initializes temporary-uninitialized slot.

> [String/UUID change made/needed]
None

Comment 30

[Hannes Verschore [:h4writer]]

Comment on attachment 8745185 [details] [diff] [review]
Re-enable bug 1263857 testcase.

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

Sure!

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741229 [details] [diff] [review]
(mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.

Crash fix that was verified by fuzzing team on 48, Beta47+

Comment 32

[Tooru Fujisawa [:arai]]

Is there any reason the patch for beta is not landed?  or it's just overlooked?