Closed Bug 839209 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox18 --- unaffected
firefox19 --- wontfix
firefox20 + fixed
firefox21 + fixed
firefox22 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: nihsanullah)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [consider backing out bug 781657] [jsbugmon:update,ignore][adv-main20+])

Attachments

(1 file)

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);
S-s due to GC-related assertion.
Whiteboard: [jsbugmon:update,bisect]
I think the assertion is in gc::MarkKind.  Marking something incorrectly sounds possibly quite bad.
Keywords: sec-critical
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
> 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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
I'm guessing Brian can own this? (cc+ dvander)
Assignee: general → bhackett1024
Naveed - this is a recent sec-critical regression with an actionable testcase and a known regressing bug. Should we consider backing out bug 781657.
Assignee: bhackett1024 → nihsanullah
Keywords: regression
Whiteboard: [jsbugmon:update] → [jsbugmon:update][consider backing out bug 781657]
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.
Whiteboard: [jsbugmon:update][consider backing out bug 781657] → [jsbugmon:update,reconfirm][consider backing out bug 781657]
(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.
Whiteboard: [jsbugmon:update,reconfirm][consider backing out bug 781657] → [jsbugmon:update,reconfirm]
Whiteboard: [jsbugmon:update,reconfirm] → [consider backing out bug 781657] [jsbugmon:update,reconfirm,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision e23e43a2c14e).
Brian, as comment 11 shows this bug still repros on tip. What architecture did you try?
Flags: needinfo?(bhackett1024)
Whiteboard: [consider backing out bug 781657] [jsbugmon:update,reconfirm,ignore] → [consider backing out bug 781657] [jsbugmon:update]
I still can't reproduce.  I've tried x64/linux, x64/darwin, and x86/darwin.
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
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.
Attachment #726704 - Flags: review?(dvander)
Attachment #726704 - Flags: review?(dvander) → review+
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
Attachment #726704 - Flags: sec-approval?
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 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.
Attachment #726704 - Flags: sec-approval? → sec-approval+
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
Attachment #726704 - Flags: approval-mozilla-beta?
Attachment #726704 - Flags: approval-mozilla-aurora?
This looks like it will apply clean to aurora/beta.
Whiteboard: [consider backing out bug 781657] [jsbugmon:update] → [consider backing out bug 781657] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1d6fe70c79c5).
https://hg.mozilla.org/mozilla-central/rev/4bb793ac828d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 726704 [details] [diff] [review]
patch

Low/no risk, verified, let's get this into our final FF20 beta.
Attachment #726704 - Flags: approval-mozilla-beta?
Attachment #726704 - Flags: approval-mozilla-beta+
Attachment #726704 - Flags: approval-mozilla-aurora?
Attachment #726704 - Flags: approval-mozilla-aurora+
Whiteboard: [consider backing out bug 781657] [jsbugmon:update,ignore] → [consider backing out bug 781657] [jsbugmon:update,ignore][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.