1404791 - stylo: Crash in geckoservo::glue::Servo_ComputedValues_EqualCustomProperties

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Chris Peterson [:cpeterson]]

This bug was filed from the Socorro interface and is 
report bp-9c36234d-3cbb-46d9-a932-316c90171002.
=============================================================

135 crash reports from 62 installations. Among Stylo-specific crash signatures, this is Stylo top crash #4.

Comment 1

[Cameron McCormack (:heycam)]

All URLs from crash reports are YouTube videos.

Comment 2

[Bobby Holley (:bholley)]

So, we're crashing when performing deep-equality on the CustomPropertiesMap hashtables.

dmajor and I dug into some of the minidumps. We're crashing when trying to extract nsIAtom::mHash from garbage memory (0xe5d5..., 0x0, or similar). This happens in HashMap::eq(), which looks like this:

self.iter().all(|(key, value)| other.get(key).map_or(false, |v| *value == *v))

So we iterate the first map, which gives us a bunch of key/value pairs, where the nsIAtom* is the key. And at least one of those pointers is garbage. This strongly suggests that the storage of at least one (k,v) pair returned from iterating the first map is uninitialized memory. This makes sense, since upon table allocation, only the hash storage is zeroed, and the (k,v) storage is left uninitialized as an optimization.

This also suggests that the crashes in bug 1385925 are indeed due to an std::HashMap bug, rather than external corruption. This also explains why bug 1403397 hasn't found any strong evidence of external memory corruption.

I could further confirm this theory by poisoning the rest of the table buffer with a unique value. It's a bit tricky that these particular crashes only happen on 57 beta (where we don't want to run diagnostic code if we can avoid it), but we may be able to see it on the other HashMap crashes we still have on Nightly.

In the mean time, Gankro is doing some HashMap auditing/fuzzing, looking for ways this might happen. Marking this as s-s, since a memory hazard in std::HashMap would be a big deal.

Comment 3

[Bobby Holley (:bholley)]

So I captured a 1.5G log of all the custom property insertions (and associated precomputed hashes) on youtube: https://www.dropbox.com/s/15wbkopvj3893tw/youtube%20custom%20property%20log.bz2?dl=0

The plan was for Gankro to feed that into a fuzzer. But I just realized that, up until 4 days ago [1], the custom properties map used the default hasher, with randomized state. So we won't be able to reliably reproduce the hashes. That said, we can at least try with the default hasher on those atoms, by just feeding the precomputed hash back into a RandomState hasher [2]. The crashes are consistent enough across installations that they can't be entirely dependent on a single random seed.

Interestingly, these crashes are _almost_ exclusively on beta (with one exception on 58a and one exception on 57a) over the past 14 days. On the other hand, the switch to PrecomputedHash on nightly only happened 4 days ago.

Still, I think it's worth getting [1] uplifted (which we were going to do anyway in bug 1385971) to see if that has any impact on the crashes.

NI Manish to request uplift on [1] (and the rest of bug bug 1385971), and NI Gankro to try fuzzing with DefaultHasher re-hashing the hashvalues in the log.

[1] https://github.com/servo/servo/pull/18679
[2] http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/servo/components/style/gecko_string_cache/mod.rs#283

Comment 4

[Aria Beingessner [:Gankra]]

Oh! I didn't realize these were going through the random hasher. Will change.

I'm new to reading aggregate crash logs, do we have any sense on if this is in any way platform-specific? I'm wondering if this might be hashmap getting miscompiled. Also: y'all were seeing this long before you forked HashMap, right?

Comment 5

[Aria Beingessner [:Gankra]]

Attachment: insertions.txt

This is the result of feeding the log through `awk '!a[$0]++'`, which the internet tells me deduplicates lines.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Alexis Beingessner [:Gankro] from comment #4)
> Oh! I didn't realize these were going through the random hasher. Will change.
> 
> I'm new to reading aggregate crash logs, do we have any sense on if this is
> in any way platform-specific?

They're almost all on windows (there is exactly one crash on mac). But that's not at all unusual, given how skewed our population is.

> I'm wondering if this might be hashmap getting miscompiled.

It's possible, but IMO unlikely. If there are any theories about specific ways it might be getting miscompiled it's straightforward to verify them.

> Also: y'all were seeing this long before you forked HashMap,
> right?

Correct.

Comment 7

[Manish Goregaokar [:manishearth]]

If we stop seeing this crash after the elimination of RandomState I wonder if it's because SipHash13 has some bad initialization values (that get hit when the random state starts at a particular value) or something. Though that shouldn't cause unsafety since Hash and Eq are safe APIs (and thus should be totally safe to mis-write) and collisions should not cause anything other than correctness problems. (Though that's worth checking, do we ever assume that hashes do not collide?)

Comment 8

[Aria Beingessner [:Gankra]]

HashMap should be perfectly capable of handling hash collisions -- although this is probably an incredibly untested path considering it's ~1/2^63 for the builtin hasher.

Comment 9

[Bobby Holley (:bholley)]

Tracking this investigation in bug 1406996.

Comment 10

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 11

[Frederik Braun [:freddy]]

(In reply to Alexis Beingessner [:Gankro] from comment #8)
> HashMap should be perfectly capable of handling hash collisions -- although
> this is probably an incredibly untested path considering it's ~1/2^63 for
> the builtin hasher.

What is the the hash function used in HashMap? 
Assuming it's not collision resistant (which functions for hash storage usually aren't), an attacker can blow this low probability out of proportion.
Does that change anything?

Comment 12

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

Still crashing in 59b14 with UAF.  https://crash-stats.mozilla.com/report/index/1b433967-a57b-4a45-a203-a93570180126

Comment 13

[Emilio Cobos Álvarez (:emilio)]

How can that be even possible?

Gecko_CalcStyleDifference always passes aIgnoreVariables = true:

  https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/layout/style/ServoBindings.cpp#378

Which means that unless it got miscompiled or what not, that function should never be called:

  https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/layout/style/nsStyleContext.cpp#165

Comment 14

[Sean Voisen (:svoisen)]

As part of layout security bug scrub, marking bugs being tracked in the Rust HashMap investigation (bug 1406996) as stalled for now.

Comment 15

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

Wontfix for 62/63.

Comment 16

[Daniel Holbert [:dholbert]]

Looks like the crash volume has been at zero here for a while. Calling this WORKSFORME.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.