Closed Bug 1402167 Opened 2 years ago Closed 2 years ago

Add diagnostic for XDR script memory corruption

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox58 --- affected

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

(Whiteboard: [js:tech-debt])

Attachments

(1 file, 1 obsolete file)

In order to help get a better understanding of what is causing corruption in Bug 1367896, I propose landing this diagnostic patch in nightly.

Based on observations of crashes in Bug 1367896, it appears that the JSScript::data buffer is somehow corrupted. This buffer allocated outside the GC. A number of crash signatures show that accessing JSScript::scopes()->vector[i] is either an outright memory violation or accessing other data. Since both |vector| and its target are in the same buffer that experiences corruption, I propose we check that |vector| still points within the buffer. The patch checks this condition in js::XDRScript (decode + encode) and js::CopyScript.
Does this seem reasonable to try?
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
Attachment #8910992 - Flags: review?(nicolas.b.pierron)
Attachment #8910992 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8910992 [details] [diff] [review]
0001-Bug-1402167-Check-JSScript-data-integrity-in-XDR-r-n.patch

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

::: js/src/jsscript.cpp
@@ +82,5 @@
> +    uint8_t* ptr = reinterpret_cast<uint8_t*>(sa->vector);
> +
> +    // Check scope data - who's pointer is stored in data region - also points
> +    // within the data region.
> +    MOZ_DIAGNOSTIC_ASSERT(ptr >= script->data &&

nit: Use a MOZ_RELEASE_ASSERT, as this assertion ensure safety, and the performance of computing it is likely neglectable compared to decoding/encoding the JSScript.

follow-up: check other ScriptData fields as well?
Attachment #8910992 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8910992 [details] [diff] [review]
0001-Bug-1402167-Check-JSScript-data-integrity-in-XDR-r-n.patch

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

This seems like a good start. My only concern is that these crashes have been so infrequent on nightlies that it might be weeks before we get any useful data unless we uplift to beta and use release asserts.
Attachment #8910992 - Flags: feedback?(kmaglione+bmo) → feedback+
Will make them release asserts and land in Nightly. I'm not going to check other fields for now since they are more involved and are unlikely to tell us more. One improvement could be to check that the scope list is sane.

If this doesn't explode on nightly, I think a beta uplift is worth doing. The scopes() check is extremely cheap and if the assertion would trip, we are at serious memory corruption and wont make it much further anyways. Before that I should probably clarify what the experiment will tell us.
Turned the DIAGNOSTIC_ASSERT into a RELEASE_ASSERT.
Attachment #8910992 - Attachment is obsolete: true
Attachment #8911328 - Flags: review+
leave-open until we get results and decide next steps.
Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddff620ec86a
Check JSScript::data integrity in XDR r=nbp
Priority: -- → P3
Whiteboard: [js:tech-debt]
This diagnostic has only been hit once in Beta. Current efforts at mitigation and diagnostics are in Bug 1418894, so I am closing this bug.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.