Closed Bug 1406220 Opened 3 years ago Closed 3 years ago

stylo: Add canaries and journaling to HashMaps

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- disabled
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: bholley, Assigned: bholley)

Details

(Keywords: sec-other, Whiteboard: [adv-main58-][post-critsmash-triage])

Attachments

(2 files)

In our ongoing quest to figure out what the heck is going on with HashMaps, I wrote up some more aggressive diagnostics to land on nightly.

First, we insert a canary between each key and value in the map. After insertions, we iterate the map, verifying all the canaries. If we ever find a bad canary, we panic.

Second, we keep a log of all the hashes inserted/removed, and include this log in the crash report. If we assume there are no collisions of full hashes (which is possible, but much less likely than collisions of hashes masked down to the table size), this will allow us to replay the conditions that led to the crash, which will hopefully allow is to reproduce it.
Group: core-security → layout-core-security
MozReview-Commit-ID: C0a5g6xMPY0
Attachment #8915788 - Flags: review?(manishearth)
MozReview-Commit-ID: 582ZiTmcvgs
Attachment #8915789 - Flags: review?(manishearth)
Attachment #8915789 - Flags: review?(dmajor)
How big is the log?
Flags: needinfo?(bobbyholley)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> How big is the log?

2 words per hashmap entry. remove() and overwriting insert() calls could technically push us over the size of the hashmap, but that seems unlikely.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa1b93d1cd3635f0d9e0414d8c6ee91ffcdd0843
Flags: needinfo?(bobbyholley)
Attachment #8915789 - Flags: review?(manishearth) → review+
Attachment #8915788 - Flags: review?(manishearth) → review+
Comment on attachment 8915789 [details] [diff] [review]
Part 2 - Add canary and journaling. v1

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

::: layout/style/ServoBindings.cpp
@@ +24,5 @@
>  #include "nsContentUtils.h"
>  #include "nsDOMTokenList.h"
>  #include "nsDeviceContext.h"
>  #include "nsIContentInlines.h"
> +#include "nsICrashReporter.h"

This probably needs an ifdef MOZ_CRASHREPORTER.
Attachment #8915789 - Flags: review?(dmajor) → review+
My question was unclear. How much will this increase the size of crash reports?
Flags: needinfo?(bobbyholley)
(In reply to Nicholas Nethercote [:njn] from comment #6)
> My question was unclear. How much will this increase the size of crash
> reports?

The only time we append the data is when we detect corruption via the canary, so it should only increase the size of a handful of crash reports.
Flags: needinfo?(bobbyholley)
Priority: -- → P2
the gecko-side changes are trivial and not interdependent, so I'm just landing them now instead of doing the coordiland dance:

https://hg.mozilla.org/integration/autoland/rev/7b16f542139d0d12722cb8dd51568ddedf81bf1e
Priority: P2 → P4
Keywords: sec-other
Group: layout-core-security → core-security-release
Whiteboard: [adv-main57-]
Whiteboard: [adv-main57-] → [adv-main58-]
Flags: qe-verify-
Whiteboard: [adv-main58-] → [adv-main58-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.