1353242 - Crash in js::ConstraintTypeSet::sweep

Status: RESOLVED INCOMPLETE

(Core :: JavaScript: GC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug was filed from the Socorro interface and is 
report bp-04d544c7-24d9-44be-a8e4-d1af52170403.
=============================================================

UAF signature.  While very few of the crashes in the function are UAFs, that (and the random addresses of the others) strongly implies that the other crashes are also UAF crashes, and if so this is a top-sec-crasher with >13000 crashes in the last week.

Crashing in    
     if (constraint->sweep(zone->types, &copy)) {

Same crash goes back to at least FF 38.x

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I'll also note that the crash volume doubled in late Jan, and again in early March (as 52 went to beta and to release, it appears).

Comment 2

[Andrew McCreight [:mccr8]]

Jonco and Sfink know about GC stuff.

Comment 3

[Daniel Veditz [:dveditz]]

The downward slope of crashes since 52 was released may represent people quitting because release is too crashy for them. Note that due to throttling, release crashes on Windows are much higher than represented in counts.

https://ashughes1.github.io/bugzilla-socorro-lens/chart.htm?s=js::ConstraintTypeSet::sweep
(get the Bugzilla Socorro Lens add-on -- it's great)

Comment 4

[Daniel Veditz [:dveditz]]

Interesting aggregations on the crash. Starting the end of January the crash rate on Windows 7 jumped up, and basically stays flat for 51 and 52 releases. With the 52 release Windows XP goes from near nothing to significantly higher than the Win 7 crashes. Windows Vista also gets a bump, though stays small. At that point 52.0.2esr dwarfs 52.0.2 normal.

Crashes on the ESR branch, like nightly, aurora, and beta, are not throttled. I think the spike in the 52 release is largely an artifact of the throttling and the fact that we moved WinXP and Vista users onto the unthrottled ESR branch. What we have here is the initial 51 Release spike that has stayed pretty consistent since.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 53. sfink says he can take a look at this later today.

Comment 6

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

Many of the crashes are reads of a freed pointer (0xffffffffe5e5e5e5) in this line:

            ObjectKey* key = oldArray[i];

Another is the second line here, an 0x4 deref (nullptr with offset 4):

        ObjectKey* key = (ObjectKey*) objectSet;
        if (!IsObjectKeyAboutToBeFinalized(&key)) {

The line in comment 0 is quite common:

        if (constraint->sweep(zone->types, &copy)) {

with crash addresses of 0xffffffffffffffff (the bogus address; need to look at minidump for actual), 0x800, 0x10, 0x1010.

Those first two are both sweeping a ConstraintTypeSet containing an ObjectSet (objectCount >= 2), and that objectSet pointer (ObjectKey**, to be specific) is partially overwritten with the jemalloc free pattern.

I will look more tomorrow, but needinfo? jandem in hopes he is more familiar with how TypeSets are manipulated.

Comment 7

[Jan de Mooij [:jandem]]

I've been adding release asserts to figure out what's going on here, see the info in bug 1333000. Everything seems to behave as it should and it looks like this involves weird bitflips. I'll think about it more.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

I realize some of our crashes in crash-stats are bitflips, but I don't see how crashes of this volume (and signature) could be bitflips.  Also, some are clear UAFs, many others seem (to me) strongly to be reallocated memory

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Randell Jesup [:jesup] from comment #8)
> but I don't see how crashes of this volume (and signature) could be bitflips.

I didn't mean *all* crashes are bitflips or all bitflips are caused by bad hardware, but we definitely see a number of bitflips in the crash reports (see bug 1333000 comment 26). I don't know if that's bad hardware or the result of memory corruption.

Comment 10

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

I notice that we have a tsan error for a data race with setObjectCount. See bug 1353902.

Unfortunately, corrupting the flags word (which includes objectCount) seems like it would be likely to cause crashes at odd addresses, which we haven't observed. Specifically, if objectCount were corrupt and appeared to be >= 2, then we would index into objectSet, an ObjectKey*, as if it were an ObjectKey** array. objectSet will have the low bit set if it is a singleton, which should show up as a crash on an odd address when we look up index i=0. So in that case, it would be a valid JSObject* with the low bit set, which would not look like 0xffffffffe5e5e5e5.

So it really seems like objectSet itself is corrupt. I'm not sure whether the flags word is mangled as well, but if it were, I would expect crashes at addresses that would be valid if their low bit were cleared.

If objectCount were too high, and we marked the objectSet, and overwrote it with a forwarded pointer that was corrupt... but why. Never mind. I can't make it fit.

Maybe I'll add some more diagnostics to remove some of this guesswork.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

It doesn't sound like we have any progress here. It is still a pretty bad crash on beta desktop and also for XP users on ESR52. 
I agree with Dan that this seems like a consistent problem since 51. So I am not considering this a blocker for 53 release. If we come up with a fix, it could be a dot release driver for 53.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

So far this doesn't look actionable. 
Steve, do you want to go ahead and add some diagnostic stuff so we may have a hope of fixing this at some point?

Comment 13

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

Critsmash triage: resolving this as incomplete as it isn't actionable and hasn't gone anywhere in the last couple of months.