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
Good catch, thanks. jonco, what's our policy on allocations? Should I just bound the size?
Flags: needinfo?(dteller) → needinfo?(jcoppeard)
(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.
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.
Summary: Large Allocations in BinAST → [BinAST] Large Allocations in BinAST
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.
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.
err, actually, the parseStringList is already removed. I'll fix other cases and newly added ones.
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.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #9013921 - Flags: review?(dteller)
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!
Attachment #9013921 - Flags: review?(dteller) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/21b67d2084a65be59d8cfd0b495276bf47b5f899 Bug 1434573 - Limit the number of Formal Parameter and Tagged Tuple. r=Yoric
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main64-]
You need to log in before you can comment on or make changes to this bug.