1347139 - Crash at startup due to low datasize limits since bug #1334933

Status: NEW

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

Description

[Landry Breuil (:gaston)]

On OpenBSD, by default the user has a 768Mb datasize ulimit, cf https://github.com/openbsd/src/blob/master/etc/etc.amd64/login.conf#L44. Since the change in bug #1334933 to pre-allocate 1Gb of memory at startup, firefox crashes at startup with allocation memory errors.

While i'm not in a position to debate the ideas behind the changes in bug #1334933 - and it's not really acceptable to say 'but you just have to raise your ulimit in login.conf' (as this would allow all user processes to allocate that much memory, and some users dont want that), i'm opening this bug so that ppl with ideas on how to avoid this new situation, while keeping default ulimits and the security (?) changes in the aforementioned bug brings - and thus fixing the crashes. I personally have no idea how this stuff works, nor am a security expert, i just dont care about all this and raise my user's datasize to 1.5Gb since forever, but some users dont want that.

Note that chromium has a wrapper script (cf https://github.com/openbsd/ports/blob/master/www/chromium/files/chrome#L4) on OpenBSD to raise its datasize limit to 760Mb at startup, and this works - i'd like to avoid that kind of ugly workarounds for the firefox port.

Comment 1

[Landry Breuil (:gaston)]

This was initially discussed/reported on the [e-mail] list, cf http://marc.info/?t=148938334800001&r=1&w=2 for reference.

Comment 2

[Sebastien Marie [:semarie]]

:jandem, is it possible to have access to #1334933 ? I saw the diff, but I would like to understand the rational for switching from "allocation on need" to "pre-allocating a big chunk at init".

for what I saw, the assumption is firefox code know better than OS how to allocate securely chunk of memory, and it seems weird to me (but I don't know all platforms and internals about memory allocation on them). So I hope to have better understanding with bug log.

Comment 3

[Luke Wagner [:luke]]

The main motivation for the up-front code reservation was so that JIT code couldn't be forced into predictable memory addresses (using heap feng shui techniques that take advantage of the deterministic allocation patterns of non-executable memory) which supports ASLR.  (It also has several practical side benefits.)

For 32-bit, we only reserve 140mb, atm.  For 64-bit, we generally assume that the process has many TB available, so a 760mb limit is, I expect, going to have an increasingly rough time across the whole browser.  For example, each WebAssembly.Memory allocation reserves a ~6gb region and so this limit will effectively disable wasm.

If it's absolutely necessary to preserve the limit, however, I suppose we could allow it to be overridden, e.g. with an env var, without too much trouble.

Comment 4

[Landry Breuil (:gaston)]

And what if the OS itself takes care of providing proper ASLR support, and the js engine just trusts it ? We've had ASLR on OpenBSD since $forever..

Comment 5

[Luke Wagner [:luke]]

Switching to a single up-front reservation solved a number of problems and it's not easy to fall back to the previous mmap-at-runtime behavior.  As I said, we could perhaps add some env var to reduce the 64-bit quota.

Comment 6

[Sebastien Marie [:semarie]]

I agree that now the switch is done, it is complex to go back. But I agree also with :landry on the fact that it is more a OS problem than an apps problem. And switching to pre-allocated area could also downgrade the security for some plaforms that take care of providing proper ALSR (as now, the application take this responsability).

For now, having a configurable way to define the 64-bit quota could be a solution. Programmatically, the total size of data segment available for the whole process is reachable using getrlimit(RLIMIT_DATA, &data) (it is a POSIX interface but I dunno for availability on Windows platform). This value could be used as an hint for defining MaxCodeBytesPerProcess. But I assume environment variable would be more simple to implement.

Thanks.

Comment 7

[Landry Breuil (:gaston)]

(In reply to Luke Wagner [:luke] from comment #5)
> Switching to a single up-front reservation solved a number of problems and
> it's not easy to fall back to the previous mmap-at-runtime behavior.  As I
> said, we could perhaps add some env var to reduce the 64-bit quota.

Even if there was a way to switch back by a build option or a runtime option, i know this wont fly because that'd be an 'untested' configuration upstream (ie not ran by the CI test suites etc).

An environment variable is imo ugly, in the sense that ppl will need a wrapper to start firefox, or configure their shell or whatever other hack. Wouldnt it be nicer if it was (yet another) about:config knob, with a minimum safeguard value ? Dunno if it's possible, as config is read *after* gecko & js engine startup.. but this way, distributors could configure "sane defaults" for every user installing the package.

I agree with sebastien here - with this move the security is downgraded on OpenBSD, because we don't benefit anymore from the advanced memory management done by the OS (ASLR, W^X, malloc canaries..), and we just have to trust whatever technique is used/reinvented in the JS engine. But i guess we'll have to live with it. Iirc webkit/blink did the same thing some years ago....

(In reply to Sebastien Marie from comment #6)
 
> For now, having a configurable way to define the 64-bit quota could be a
> solution. Programmatically, the total size of data segment available for the
> whole process is reachable using getrlimit(RLIMIT_DATA, &data) (it is a
> POSIX interface but I dunno for availability on Windows platform). This
> value could be used as an hint for defining MaxCodeBytesPerProcess. But I
> assume environment variable would be more simple to implement.

Using getrlimit/setrlimit wouldnt work as-is, because the browser needs more memory than just the JS engine.

Comment 8

[Luke Wagner [:luke]]

Right now the MaxCodeBytesPerProcess is controlled by an #if (140mb for 32-bit, 1gb for 64-bit).  So an even simpler option that avoids any wrapper script could be to just add another condition to the #if so that OpenBSD uses the same quota as 32-bit.  Then it wouldn't be an untested configuration.

(In reply to Landry Breuil (:gaston) from comment #7)
> because we don't benefit anymore from the advanced memory
> management done by the OS (ASLR, W^X, malloc canaries..)

FF still ensures W^X regardless of this change.  Also, malloc() wasn't being used to allocate executable code, executable code was allocated/pooled/reused in big mmap'ed chunks, so not entirely different than now.

Comment 9

[Sebastien Marie [:semarie]]

I am currently running with 128 Mo of preallocation without particular problems. But my use of firefox is generally without javascript (or with opt-in, so few tabs with javascript at the same time). I am waiting for some feedback from others users.

Thanks.

Comment 10

[Landry Breuil (:gaston)]

Fwiw, we've "fixed" this by patching MaxCodeBytesPerProcess to be 128Mb in firefox 52.0.1, esr 52.0.1 and thunderbird 45.8.0. I'm just concerned that using this value might (?) lower randomness of address space for the js engine, thus defeating the fix/security added from #1334933 ? Or 640Mb/1Gb was totally arbitrary ?

Comment 11

[Sebastien Marie [:semarie]]

From my understanding the randomness shouldn't be affected while a pointer real address isn't leaked. The base address of the memory block is still randomly allocated on heap.

The affected part could be allocation inside the block as the search space is smaller (and now, it is the js-engine that take responsability of proper ASLR - in a smaller area than the kernel was able to do). But unused memory (inside the block) has PROT_NONE property (neither readable, writable or executable) and it is enforced by kernel.

External confirmation would be appreciate too.

Comment 12

[Jan de Mooij [:jandem]]

Yes changing to 128mb should be fine, but you're more likely to see OOMs from running out of executable memory (especially with WebAssembly).

Comment 13

[Landry Breuil (:gaston)]

So far all the WASM demo i've seen were heavy GL stuff, and WebGL support isnt totally working on OpenBSD (No WebGL2, never found time to dig into that particular issue) - so i guess we'll live with it for now.

Comment 14

[Hannes Verschore [:h4writer]]

Can I assume (given the last comment) that we can close this as WFM for now?

Comment 15

[Landry Breuil (:gaston)]

Well, not really, to close this bug we'd have to upstream and uplift a variation of https://github.com/openbsd/ports/blob/master/www/mozilla-firefox/patches/patch-js_src_jit_ProcessExecutableMemory_cpp ...

Comment 16

[Landry Breuil (:gaston)]

Chunk of code was moved to ProcessExecutableMemory.h in https://hg.mozilla.org/mozilla-central/rev/23e4839619c8

Comment 17

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

Comment 18

[Jan de Mooij [:jandem]]

BSD specific AFAICS so this does not have to be P1.

Comment 19

[Landry Breuil (:gaston)]

(In reply to Jan de Mooij [:jandem] from comment #18)
> BSD specific AFAICS so this does not have to be P1.

Well... linux distros can also set systemwide limits for processes, what if a specific linux distro decides to set the systemwide limit to less than 1Gb ? To me, this is not 'BSD specific'.

Comment 20

[Landry Breuil (:gaston)]

Fwiw, we've been running fine with a default of 140Mb hardcoded for the past two years. I'd like to eventually get rid of the patch we're carrying for this, so how about upstreaming a variation of min(getrlimit(RLIMIT_DATA)/5, 1Gb), or with a better heuristic ? And eventually apply it to all unix platforms ?

Recently started an unmodified nightly build, and it segfaulted right away because mmap failed with ENOMEM, even with ulimit -d defaulting to 1.5Gb, so that memory consumption is a bit worrying.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Landry Breuil (:gaston) from comment #20)

Fwiw, we've been running fine with a default of 140Mb hardcoded for the past two years. I'd like to eventually get rid of the patch we're carrying for this, so how about upstreaming a variation of min(getrlimit(RLIMIT_DATA)/5, 1Gb), or with a better heuristic ? And eventually apply it to all unix platforms ?

What's been nice about the current constant value is that we can use it in static_asserts and to compute other constants, for example MaxCodePages. It also makes things easier for us to reason about. I'm a bit wary of adding a different notion of "non-constant, actually reserved size" just for OpenBSD.

What about hard-coding 140Mb on 64-bit platforms with an OpenBSD ifdef?

Comment 22

[Lars T Hansen [:lth]]

OpenBSD is probably at the extreme here but I've seen other systems with very low rlimit values on 64-bit (in the 8GB range of address space), which will make them fail to deal with WebAssembly content at all. In response to that problem we've started to choose a compilation strategy for wasm that depends on the rlimit value -- if it's quite low, we choose explicit bounds checks and a small memory reservation even on 64-bit systems. I know that that's a different problem than what's being debated here, but we could usefully be more adaptive here than we are. (Not saying the ifdef solution isn't the best one, just that OpenBSD is unlikely to be the only system with problems.)

Comment 23

[Landry Breuil (:gaston)]

(In reply to Jan de Mooij [:jandem] from comment #21)

(In reply to Landry Breuil (:gaston) from comment #20)

Fwiw, we've been running fine with a default of 140Mb hardcoded for the past two years. I'd like to eventually get rid of the patch we're carrying for this, so how about upstreaming a variation of min(getrlimit(RLIMIT_DATA)/5, 1Gb), or with a better heuristic ? And eventually apply it to all unix platforms ?

What's been nice about the current constant value is that we can use it in static_asserts and to compute other constants, for example MaxCodePages. It also makes things easier for us to reason about. I'm a bit wary of adding a different notion of "non-constant, actually reserved size" just for OpenBSD.

What about hard-coding 140Mb on 64-bit platforms with an OpenBSD ifdef?

That's what we've been shipping since 2 years in
https://github.com/openbsd/ports/blob/master/www/mozilla-firefox/patches/patch-js_src_jit_ProcessExecutableMemory_h - i can of course upstream that but to me that's a bandaid/hack - and yeah as :lth satys other operating systems might apply rlimits.