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.

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?

(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.

This has been an issue since we since we started moving scripts in bug 1122579.

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.

sec-approval+ for trunk. We'll want a patch for aurora as well.

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.