Closed Bug 1263857 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- verified

People

(Reporter: gkw, Assigned: arai)

References

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(6 files, 1 obsolete file)

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.
Summary: Assertion failure: (ptrBits & 0x7) == 0, at /Users/skywalker/shell-cache/js-dbg-64-dm-clang-darwin-29d5a4175c8b/objdir-js/dist/include/js/Value.h:854 → Assertion failure: (ptrBits & 0x7) == 0, at dist/include/js/Value.h:854
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?
Blocks: 887016
Flags: needinfo?(arai.unmht)
The testcase in comment 1 was reduced from a crash at js::gc::MaybeForwarded.
Crash Signature: [@ js::gc::MaybeForwarded]
Summary: Assertion failure: (ptrBits & 0x7) == 0, at dist/include/js/Value.h:854 → Crash [@ js::gc::MaybeForwarded] or Assertion failure: (ptrBits & 0x7) == 0, at dist/include/js/Value.h:854
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.
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?
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8740390 - Flags: review?(hv1989)
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 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.
Attachment #8740390 - Flags: review?(hv1989)
Thank you for your feedback :)
Implemented (2).
Attachment #8740390 - Attachment is obsolete: true
Attachment #8740698 - Flags: review?(hv1989)
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!
Attachment #8740698 - Flags: review?(hv1989) → review+
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.
Attachment #8740698 - Flags: sec-approval?
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).
Attachment #8740698 - Flags: sec-approval? → sec-approval+
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.
Attachment #8741229 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/c77b965d8c74
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8741229 - Flags: approval-mozilla-aurora?
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Depends on: 1265187
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)
Attachment #8741229 - Flags: approval-mozilla-aurora?
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.
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)
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?
Flags: needinfo?(sphink)
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 "!"
Flags: needinfo?(sphink)
Flipped the condition.
Attachment #8742314 - Flags: review?(sphink)
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.)
Attachment #8742314 - Flags: review?(sphink) → review+
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
Attachment #8741229 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e9e16e010ff25f3fd1ff2c2911dee4d1b8b0d2
Bug 1263857 - (followup) Disable windows crash reporter on automated tests. r=sfink
Re-enabled the testcase (this is actually for bug 1265187, but posting here as this is still s-s)
Attachment #8745185 - Flags: review?(hv1989)
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
Attachment #8741229 - Attachment description: (mozilla-aurora) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub. → (mozilla-beta) Initialize the slots of the match result object before creating properties in generateRegExpMatcherStub.
Attachment #8741229 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8745185 [details] [diff] [review]
Re-enable bug 1263857 testcase.

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

Sure!
Attachment #8745185 - Flags: review?(hv1989) → review+
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+
Attachment #8741229 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is there any reason the patch for beta is not landed?  or it's just overlooked?
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.