1013001 - Assertion failure: ptr->isTenured(), at jit/shared/Assembler-shared.h:189

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 41a54c8add09 (run with --fuzzing-safe --ion-eager):


function second() {
  var arr = {};
  var s = 'a|b|c';
  return s.replace(/[a-z]/g, function(a) { return arr[a]; }, 'g');
}
Object.defineProperty(
  Object.prototype, "b", {
    get: (function() {
            "use asm"
            return {}
        })
  }
);
second();

Comment 1

[Christian Holler (:decoder)]

Another issue found with the simulator build, marked as s-s because it involves the Assembler and the assertion sounds GC-related.

Comment 2

[Andrew McCreight [:mccr8]]

Is there somebody who can look at this Jandem?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Is there somebody who can look at this Jandem?

Hm this could be a duplicate of bug 1023158, let's try this again once that bug is fixed...

Comment 4

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 76b55c0850ca).

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
Due to skipped revisions, the first good revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a0dd5a83ba36
user:        Jan de Mooij
date:        Thu Jul 24 11:56:43 2014 +0200
summary:     Bug 1031529 part 2 - Remove JS_THREADSAFE #ifdefs everywhere. r=bhackett

changeset:   https://hg.mozilla.org/mozilla-central/rev/6426fef52f51
user:        Jan de Mooij
date:        Thu Jul 24 11:56:45 2014 +0200
summary:     Bug 1031529 part 3 - Step defining JS_THREADSAFE, remove --disable-threadsafe. r=glandium

This iteration took 479.049 seconds to run.

Comment 6

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

Still happens with rev 7fc96293ada8 with these flags: --fuzzing-safe --ion-eager --no-threads

function second() {
  var arr = {};
  var s = 'a|b|c';
  return s.replace(/[a-z]/g, function(a) { return arr[a]; }, 'g');
}
Object.defineProperty(
  Object.prototype, "b", {
    get: (function() {
            "use asm"
            return {}
        })
  }
);
second();

Comment 7

[Terrence Cole [:terrence]]

Attachment: bug_1013001-v0.diff

Sorry I didn't see this bug sooner, as the fix is straightforward. Please do NI me on any like this in the future so that I don't miss them in the tide of bugmail.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8505876 [details] [diff] [review]
bug_1013001-v0.diff

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

Looks good and thanks for fixing this, but see the question below.

::: js/src/jit-test/tests/gc/bug-1013001.js
@@ +1,1 @@
> +function second() {

If it's really a sec-high we should probably land without the testcase.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1178,5 @@
>      void pushValue(const Value &val) {
>          jsval_layout jv = JSVAL_TO_IMPL(val);
>          push(Imm32(jv.s.tag));
>          if (val.isMarkable())
> +            push(ImmMaybeNurseryPtr(reinterpret_cast<gc::Cell *>(val.toGCThing())));

Don't we need this on other platforms too?

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8505876 [details] [diff] [review]
bug_1013001-v0.diff

On further investigation, this is simply incomplete -- the same path is buggy in x86.

Comment 10

[Terrence Cole [:terrence]]

Attachment: bug_1013001-v0.diff

This re-implementation of ImmMaybeNurseryPtr strictly enforces the membrane, catching the misuse on the other two platforms as well as simplifying the rest of the code significantly.

This patch gives visibility of the raw pointer value in an ImmMaybeNurseryPtr to only AssemblerShared. This should make it impossible to do anything at all with a nursery pointer without first calling noteMaybeNurseryPtr. Between this, the isTenured assertion in ImmGCPtr, and the onMainThread assertion, it should now be basically impossible to use a nursery pointer incorrectly in the assembler in debug builds.

I've built and tested this with x86, x64, and arm-simulator builds. It will probably break mips and arm64 builds when we land it, but fixing these will be trivial.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8506478 [details] [diff] [review]
bug_1013001-v0.diff

Nope, it looks like this doesn't actually close the hole discussed bug 1023158. Next time's the charm.

Comment 12

[Terrence Cole [:terrence]]

Attachment: bug_1013001-v2.diff

To address the case of writeValueRelocation(Value), I (1) moved the implementation from x86-shared.h to x64.h, as it isn't used on x86, and (2) handled the IsInsideNursery case manually. This is less bulletproof than the other changes here, but I think is as good as we're going to get without an enormous amount of work wrapping Value in an ImmGCValue/ImmMaybeNurseryValue equivalent.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

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

Great refactoring. Thanks for fixing this!

::: js/src/jit/shared/Assembler-shared.h
@@ +223,2 @@
>  // Used for immediates which require relocation.
> +class ImmGCPtr

I wonder if we should rename this to ImmTenured(GC)Ptr or something as a followup?

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not easily, although the fuzzing testcase is pretty small.

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

Which older supported branches are affected by this flaw?
Back to 33.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be fairly easy to backport and low risk.

How likely is this patch to cause regressions; how much testing does it need?
It's mostly a type-system change, so it's fairly unlikely to cause regressions or need significant testing.

Comment 15

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

Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

Sec-approval+. Please nominate for Aurora and Beta as well so we can fix it there (or make new patches for these if the current one doesn't apply cleanly and nominate them).

Comment 16

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5df7f0bf77fc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5df7f0bf77fc

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Terrence Cole [:terrence]]

Attachment: beta-uplift-bug1013001.diff

Approval Request Comment
[Feature/regressing bug #]: GGC.
[User impact if declined]: A potentially exploitable sec-high in release browsers.
[Describe test coverage new/current, TBPL]: On m-i/m-c for several weeks.
[Risks and why]: Low: there isn't much that can go wrong other than build failures.
[String/UUID change made/needed]: None.

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 8507200 [details] [diff] [review]
bug_1013001-v2.diff

Approval Request Comment
Same as for beta.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/afef8d8afd37
https://hg.mozilla.org/releases/mozilla-beta/rev/c94fc6b83daa

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx34
JSBugMon: This bug has been automatically verified fixed on Fx35