Closed Bug 1438057 Opened 6 years ago Closed 6 years ago

Possibly restrict random executable address on mac to a single 32-bit region

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED INVALID

People

(Reporter: erahm, Unassigned)

Details

The Chromium devs found an internal leak in OSX's kernel that is exacerbated by mmaping memory at random addresses [1]. We have similar code so we might want to take precautions as well.

From what I can tell we do mmap executable JIT code at a random address [2], but it also seems like this is only done once from a static instance of `ProcessExecutableMemory`.

It would be great if someone can confirm my interpretation. Regardless we might want to fix this to avoid future issues if we end up using `ProcessExecutableMemory` beyond a singleton.

Additionally we should audit the rest of the codebase. A cursory glance lead me to the JIT allocations.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=700928#c129
[2] https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/js/src/jit/ProcessExecutableMemory.cpp#263-284
[3] https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/js/src/jit/ProcessExecutableMemory.cpp#610-614
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #0)
> It would be great if someone can confirm my interpretation.

We allocate a single chunk of memory per process, at a random address, then we allocate executable pages from that (using mmap to commit/decommit).

> Regardless we
> might want to fix this to avoid future issues if we end up using
> `ProcessExecutableMemory` beyond a singleton.

If you think the code is okay right now, I don't think we should worry about that because I don't expect this to change anytime soon. We could put a scary comment next to the singleton declaration or increment/compare a counter in the ProcessExecutableMemory constructor and call it a day.
-> P1 because this should be resolved soon, one way or another
Priority: -- → P1
Re-reading, this doesn't seem to be a bug we ever actually had.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.