Closed Bug 1268083 Opened 4 years ago Closed 4 years ago
Global Entry for baseline jitcode is not updated correctly
JitcodeGlobalTable lookup methods return a copy of the JitcodeGlobalEntry, so when these are marked then any moved pointers will not be updated. As far as I can tell this only affects the script pointer in BaselineEntry.
I changed JitcodeGlobalTable::lookupInfallible to return a reference to the entry rather than copying it, and updated callers.
Attachment #8746060 - Flags: review?(jdemooij)
Comment on attachment 8746060 [details] [diff] [review] bug1268083-jitcode-map Review of attachment 8746060 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, good catch. ::: js/src/jit/JitcodeMap.h @@ +1029,5 @@ > return skiplistSize_ == 0; > } > > bool lookup(void* ptr, JitcodeGlobalEntry* result, JSRuntime* rt); > bool lookupForSampler(void* ptr, JitcodeGlobalEntry* result, JSRuntime* rt, Should we change these two methods to return a pointer (null on failure)? Or rename to lookupCopy?
Attachment #8746060 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #2) > Should we change these two methods to return a pointer (null on failure)? Or > rename to lookupCopy? Good call, I'll try making them return pointers.
Kannan, in this code in ProfilingFrameIterator::getPhysicalFrameAndEntry: jit::JitcodeGlobalTable* table = rt_->jitRuntime()->getJitcodeGlobalTable(); if (hasSampleBufferGen()) table->lookupForSampler(returnAddr, entry, rt_, sampleBufferGen_); else table->lookupInfallible(returnAddr, entry, rt_); I just wanted to check, is lookupForSampler infallible too? It's assumed to succeed here.
Updated patch with review comments.
Comment on attachment 8749118 [details] [diff] [review] bug1268083-jitcode-map v2 [Security approval request comment] How easily could an exploit be constructed based on the patch? Very difficult. I haven't see a crash due to this but it is possible. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? 48 is affected. If not all supported branches, which bug introduced the flaw? Bug 1122579. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Attachment #8749118 - Flags: sec-approval?
sec-approval+ for trunk. We'll want a patch for aurora as well.
Attachment #8749118 - Flags: sec-approval? → sec-approval+
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Approval Request Comment [Feature/regressing bug #]: Bug 1259180 [User impact if declined]: Possible crash / vulnerability. [Describe test coverage new/current, TreeHerder]: On m-c since 6th May. [Risks and why]: Low. [String/UUID change made/needed]: None.
Attachment #8751212 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.