1962026 - Crash [@ EnterIon] due to mmap taking a bite out of the stack and interfering with fuzzing

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Steve Fink [:sfink] [:s:]]

Forked bug 909094. The problem is regarded as a fuzzblocker. This bug intends to sidestep that problem on fuzzing builds only by reserving the full potential stack in advance to prevent mmap from using any of it. When previously attempted on all builds, this caused worse problems than the original issue. So the strategy here is just get it out of the way of fuzzing; if a test case would have run into this, it will most likely OOM first. In no cases will it be able to crash anymore.

(It remains to be worked out exactly what this means for non-main threads.)

Comment 1

[Steve Fink [:sfink] [:s:]]

I've implemented a version of this that seems to be working in the shell, at least. I take the current stack pointer and map the whole allowed JS stack space on startup. The new mapping will clobber a not yet used portion of the existing stack, which is fine. It behaves as a regular stack for C++ frames (as in, if you push onto the stack more than is mapped, it will automatically expand the stack and move the guard page down, at least until it hits another mapping.)

Some caveats:

• In /proc/(pid)/maps, only the old mapping has the "[stack]" annotation. That is mostly cosmetic, though it might confuse tools that use that label to identify the stack mapping.
• The new mapping could be changed to be executable, whereas the original mapping cannot. (As in, mprotect could add the PROT_EXEC bit.) This should be irrelevant for fuzzing, it would only be an issue if an attacker were to exploit it. (Though if an attacker can manage to get a rogue mprotect call, they've probably already broken in enough that it doesn't matter.) Apparently the Right Way to fix this is sandboxing.
• This moves some memory from VmStk to VmData in proc/(self)/maps. I think that's also based on the (flawed) heuristic that labels one mapping "[stack]".
• This bloats up the reported RSS usage by some amount. I'm not sure why; it doesn't happen immediately.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: (secure)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/07c4fb6ddb8c
https://hg.mozilla.org/integration/autoland/rev/d9800d753b74
On Linux fuzzing builds, reserve the full JS stack space on main thread startup r=decoder,jonco

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9800d753b74

Given that this only affects fuzzing builds, I don't see any need to uplift this.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8526d2585980
https://hg.mozilla.org/integration/autoland/rev/de97b3b2a669
Revert "Bug 1962026 - On Linux fuzzing builds, reserve the full JS stack space on main thread startup r=decoder,jonco" on request

Comment 6

[Steve Fink [:sfink] [:s:]]

This looks like it didn't work because my "main stack only" check appears to have not worked, causing bug 1977272.

The PAC (ProxyAutoConfig) JSRuntime is started from a thread as the "main" runtime (parentRuntime=nullptr). I used this as a signal that we are on the process's main thread, and mmapped a big JS stack. That meant I clobbered whatever came before the PAC thread's stack -- causing delightfully weird crash stacks, fwiw. In the one I ended up looking at, I zeroed out a vtable and caused the next virtual method call on a totally different thread to crash. rr is truly a great tool.

Given that this is already linux-specific, I think maybe I can get away with just doing getpid() == gettid() and not trying to "fix" JS initialization stuff.

Comment 7

[Steve Fink [:sfink] [:s:]]

I have a test that changed behavior in linux32 (js/src/jit-test/tests/gc/bug-1459860.js). It used to get an over-recursion error. Now it's failing in pthread_create because the VM space is exhausted. Which is odd; I'm mapping an additional amount just above 1MB, and each thread is increasing the VmSize by 8MB, so the 1MB shouldn't matter much at all. But the VmSize is indeed climbing rapidly to 4GB and then failing.

...aw crud, the same thing is happening without my patches. Why... what... argh.

Comment 8

[Steve Fink [:sfink] [:s:]]

Oh! This appears to be bug 1978275. I should check my needinfos more frequently.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a08f5aa99c1b
https://hg.mozilla.org/integration/autoland/rev/41742cc635fc
On Linux fuzzing builds, reserve the full JS stack space on main thread startup r=decoder,jonco

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/41742cc635fc

Comment 13

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox142 to wontfix.

For more information, please visit BugBot documentation.