1434573 - [BinAST] Large Allocations in BinAST

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Christian Holler (:decoder)]

There are several places in BinAST where external data values flow directly into allocators, causing gigabytes of memory to be requested by a single allocation. We should think about limits for this type of allocation to make the code more robust, also helping automated testing.

I've identified two places so far:

https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/js/src/frontend/BinSource.cpp#402

https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/js/src/frontend/BinTokenReaderTester.cpp#330

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Good catch, thanks.

jonco, what's our policy on allocations? Should I just bound the size?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
I don't know whether we have a policy on this.

One thing you could do would be to sanity check length values based on the remaining bytes left in the input: trying to create a list that would be impossible to populate given how much data you have left is definitely an error.

Maybe we should impose hard limits too, I'm not sure.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

That was my initial thought, but that may not scale up to the version that performs on-the-fly decompression. I'll need to check if we can have the file size information easily before decompressing from gzip, for instance, but I'm not sure.

Comment 4

[Christian Holler (:decoder)]

Marking as fuzzblocker, we cannot continue testing BinJS without adding some kind of limits here that prevent the instant OOMs. I had to stop BinJS fuzzing and server-side reducer jobs for it.

Comment 5

[Tooru Fujisawa [:arai]]

given those places are just reserving the memory space, we can use some small limit and reallocate later if necessary.

I'm not sure the best number for it tho, I'll add the limitation for now, in order to solve the blocker.
we could investigate the common case later and modify it to use the appropriate limit.

Comment 6

[Tooru Fujisawa [:arai]]

err, actually, the parseStringList is already removed.
I'll fix other cases and newly added ones.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Limit the number of Formal Parameter and Tagged Tuple.

so, I located the following 2 places:
  * BinASTParser::parseListOfAssertedMaybePositionalParameterName
    positionalParams array is resized with list length
    this should check the length with ARGNO_LIMIT = 65535
  * BinTokenReaderTester::enterTaggedTuple
    fields array is reserved with the number of tagged tuple fields
    just added the max = 32.  no interface has more than 32 fields now.
    we could reduce it, but I think simple format will be removed near future (is it correct?), to we don't have to worry about the exact number.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9013921 [details] [diff] [review]
Limit the number of Formal Parameter and Tagged Tuple.

Review of attachment 9013921 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21b67d2084a65be59d8cfd0b495276bf47b5f899
Bug 1434573 - Limit the number of Formal Parameter and Tagged Tuple. r=Yoric

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/21b67d2084a6