[BinAST] Large Allocations in BinAST

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: decoder, Assigned: arai)

Tracking

(Blocks 2 bugs, {sec-want})

Trunk
mozilla64
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [fuzzblocker][adv-main64-])

Attachments

(1 attachment)

Reporter

Description

a year ago
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
Flags: needinfo?(dteller)
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.
Flags: needinfo?(jcoppeard)
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.
Priority: -- → P3
Summary: Large Allocations in BinAST → [BinAST] Large Allocations in BinAST
Reporter

Comment 4

8 months ago
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.
Flags: needinfo?(arai.unmht)
Whiteboard: [fuzzblocker]
Assignee

Comment 5

8 months ago
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.
Assignee

Comment 6

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

Comment 7

8 months ago
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
Flags: needinfo?(arai.unmht)
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+
Assignee

Comment 9

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

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21b67d2084a6
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main64-]
Assignee

Updated

5 months ago
Duplicate of this bug: 1431077
You need to log in before you can comment on or make changes to this bug.