Harden ICStub data structures

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
2 months ago

People

(Reporter: tcampbell, Assigned: iain)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

Last year
Bug 1192988 has a number of crashes where we bad ICStub data. Some of these look like single-bit errors, but others look like more nonsense.

We should consider adding magic value bits to ICStubs. I would presume we are already poisoning the data on free since they are GCThings.

Nightly hits the ICStub::traceCode crash 3-5x a day so a MOZ_DIAGNOSTIC_ASSERT is probably fine to begin with. Things like Bug 1250964 manifested in similar ways and had real bugs. It is probably worth a quick fuzz before landing anything though.
Priority: P1 → P3
Reporter

Comment 1

3 months ago

Iain, can you take a look at maybe doing this so we advance Bug 1192988 at least a little bit. Likely memory corruption, but more predicatable crashes is better.

Assignee: tcampbell → nobody
Flags: needinfo?(iireland)
Priority: P3 → P2
Assignee

Comment 3

3 months ago

The most interesting type signature for our purposes is this callstack:

js::jit::ICScript::trace(JSTracer*)
js::jit::ICEntry::trace(JSTracer*)
js::jit::ICStub::trace(JSTracer*)

All of the recent Nightly failures in the parent bug are of this form. Skimming through those crashes, I see a reasonable mix of chips, operating systems, and 32-bit vs 64-bit.

The null hypothesis here is that there's no bug specific to ICStubs, and we're just reading garbage memory as if it were an ICStub because an earlier pointer went awry. In an attempt to disprove this hypothesis, I'm grabbing 7 unused kind bits from ICStub (we use 13 bits to represent a value that cannot exceed 6 bits), filling them with a magic value, and then verifying that the magic value is correct before tracing with a diagnostic assert.

If there is an ICStub-specific problem, we should continue seeing this crash. If the bug is a bad ICStub pointer, the assertion will fire instead.

ICEntry consists of a pointer and a 32-bit offset. On 32-bit architectures, that packs nicely, and we have no free space. On 64-bit architectures, we have a full 32 bits of padding to work with. I'm not adding an equivalent magic check for ICEntry yet; if we decide that we want one, we might want to only enable it on 64-bit.

Assignee

Updated

3 months ago
Flags: needinfo?(iireland)
Reporter

Comment 5

2 months ago

This is pretty generic stability work and doesn't need to be hidden, especially now that it is in progress.

Group: javascript-core-security
Keywords: sec-want
Reporter

Updated

2 months ago
Assignee: nobody → iireland
Status: NEW → ASSIGNED

Comment 6

2 months ago
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ae898cd5fa7
Add magic bits to ICStub and verify them before tracing r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/dc07d7bcb957
Add magic bits to ICEntry on 64-bit and verify them before tracing r=tcampbell
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.