Closed Bug 1415852 Opened 6 years ago Closed 6 years ago

Unconditionally poison chunk trailer on free

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

To help catch use-after-free bugs we should unconditionally poison the chunk trailer for free chunks.

The trailer (ChunkTrailer) stores things like the associated runtime that are used when working with cells in that chunk.  Poisoning these values could show up UAF bugs much sooner.  There shouldn't be any noticeable performance impact so we should do this all the time.

This could happen in GCRuntime::recycleChunk().
Priority: -- → P3
Assignee: nobody → jcoppeard
Jonco, since it's a good first bug, can I take this? :P

Thanks
Attached patch bug1415852-poison-chunk-trailer (obsolete) — Splinter Review
Well I already wrote the patch.  There are some test failures with this I'm looking into at the moment.
Depends on: 1419373
Rebased patch to unconditionally poison chunk trailer on free.
Attachment #8969629 - Flags: review?(sphink)
Attachment #8928508 - Attachment is obsolete: true
Comment on attachment 8969629 [details] [diff] [review]
bug1415852-poison-chunk-trailer v2

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

Uh... weird question, but did we have this at some point? Because I went to add this to the mrgiggles knowledge base of magic numbers, and found this already there:

        '8b': '''\                                                                                                                                                
0x8b is JS_FREED_CHUNK_PATTERN, the poison value written to the trailer of freed chunks                                                                           
<...2sec...>this will be accessed when looking up things like runtime, store buffer address, or ChunkLocation                                                     
''',

but I don't see it in the current code, and I can't find it with hg grep.
Attachment #8969629 - Flags: review?(sphink) → review+
Oh, never mind. I looked at when I added it:

changeset:   236:206e54983146
user:        Steve Fink <sfink@mozilla.com>
date:        Thu Nov 16 16:49:58 2017 -0800
summary:     Document 0x8b8b8b8b in anticipation of jonco landing bug 1415852
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5dc2aea493
Unconditionally poison chunk trailer on free r=sfink
https://hg.mozilla.org/mozilla-central/rev/dd5dc2aea493
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.