839209 - Assertion failure: kind == GetGCThingTraceKind(*thingp), at gc/Marking.cpp:353

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision d55669a02834 (no options required):


var N = 100*1000;
function build(N) {
  var chainTop = null;
  for (var i = 0; i != N; ++i) {
    var f = Function('some_arg'+i, ' return /test/;');
    var re = f();
    (f)     = chainTop;
  }
}
var chainTop = build(N);

Comment 1

[Christian Holler (:decoder)]

S-s due to GC-related assertion.

Comment 2

[Andrew McCreight [:mccr8]]

I think the assertion is in gc::MarkKind.  Marking something incorrectly sounds possibly quite bad.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is the bisect robot not working? I tried bisecting myself, but for some reason I can't build old revisions anymore because of a python error.

Traceback (most recent call last):
  File "../config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "../config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "../config/expandlibs_exec.py", line 26, in <module>
    from expandlibs import ExpandArgs, relativize, isObject, ensureParentDir, ExpandLibsDeps
  File "/home/billm/mozilla/in4/js/src/config/expandlibs.py", line 31, in <module>
    import expandlibs_config as conf
  File "/home/billm/mozilla/in4/js/src/config/expandlibs_config.py", line 5, in <module>
    def normalize_suffix(suffix):
ImportError: No module named buildconfig

Comment 4

[Jesse Ruderman]

> for some reason I can't build old revisions anymore because of a python error

> ImportError: No module named buildconfig

Try blowing away all *.pyc files each time you update.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   111162:9a5191dfae8d
user:        Brian Hackett
date:        Tue Oct 23 09:20:56 2012 -0700
summary:     Keep the interpreter stack synced for GC scanning, bug 781657. r=billm

This iteration took 92.237 seconds to run.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm guessing Brian can own this? (cc+ dvander)

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(meant to cc Naveed)

Comment 8

[Alex Keybl [:akeybl]]

Naveed - this is a recent sec-critical regression with an actionable testcase and a known regressing bug. Should we consider backing out bug 781657.

Comment 9

[Brian Hackett [Laid off!]]

WFM on tip.  Lots of stuff depends on bug 785167 and it can't be backed out.  Having more information about the original failure would be good; there were a couple regressions from bug 781657 that only manifested as null dereferences, and may have fixed this.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> WFM on tip.  Lots of stuff depends on bug 785167 and it can't be backed out.
> Having more information about the original failure would be good; there were
> a couple regressions from bug 781657 that only manifested as null
> dereferences, and may have fixed this.

What's the discoverability/exploitability of this bug? We're trying to determine whether this should still be made a priority ahead of release.

If so, we can loop in Matt Wobensmith (security QA) and others to find the fix range.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision e23e43a2c14e).

Comment 12

[Christian Holler (:decoder)]

Brian, as comment 11 shows this bug still repros on tip. What architecture did you try?

Comment 13

[Brian Hackett [Laid off!]]

I still can't reproduce.  I've tried x64/linux, x64/darwin, and x86/darwin.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: patch

OK, was able to reproduce with help from decoder.  The problem is an interaction of a couple things:

- JM does not guarantee a correctly synced payload for dead values that can only be seen by the GC, in cases where the value's type is not traceable and should be ignored by the GC.

- Due to a quirk with how Value operations are implemented on x64, values with a null type tag but a non-null payload are treated by the GC as traceable, and will be traced as if they are strings.  These values will not be treated as traceable on x86.

Fixing the quirk might be nice, but doing that just to placate JM's torn values doesn't seem right.  This patch just forces JM to keep null values fully synced.

Comment 15

[Brian Hackett [Laid off!]]

Comment on attachment 726704 [details] [diff] [review]
patch

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

very hard

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

no

Which older supported branches are affected by this flaw?

19+

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

bug 781657

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

trivial

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

none

Comment 16

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

Adding Release Management to discuss when we want to give sec-approval and take this. I do see that it is a pretty tiny patch.

Comment 17

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

Comment on attachment 726704 [details] [diff] [review]
patch

Sec-approval+. Please prepare patches for Aurora and Beta too and nominate them ASAP so maybe we can release this fix.

Comment 18

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb793ac828d

Comment 19

[Brian Hackett [Laid off!]]

Comment on attachment 726704 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 781657
User impact if declined: potential exploit
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): none

Comment 20

[Brian Hackett [Laid off!]]

This looks like it will apply clean to aurora/beta.

Comment 21

[Christian Holler (:decoder)]

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

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4bb793ac828d

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 726704 [details] [diff] [review]
patch

Low/no risk, verified, let's get this into our final FF20 beta.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0145d8911247
https://hg.mozilla.org/releases/mozilla-beta/rev/3ed701c01eb2