fix oom when iterating binding opcodes #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Found this bug while fuzzing the library.
If the macho is malformed and has a rebase opcode of
`RebaseOpcode::RebaseAndSkipping { times: 0xffffffff, skip: 0x4 }`,
the iterator will try to process the opcode in a single call to `Iterator::next()`,
pushing all the genearted symbols to an internal VecDeque, only to return them
one at a time to the user on subsequent calls.
This is bad because when `times` is an absurdly high number, the iterator will
cause an OOM trying to push all the symbols to its VecDeque, and the user has
no control on this behaviour.
Change the implementation to be closer to what a proper generator would do,
by remembering which instruction we were processing last time, restarting from
where we left off, and decrementing `times` every time we yield a value. This
removes the need for allocations entirely.
This does not fundamentally solve the issue, but at least now the iterator does
not OOM when calling `next()`, and the user can use `Iterator::take(limit)`
to avoid pulling an (almost) infinite stream of symbols.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found this bug while fuzzing the library.
If the macho is malformed and has a rebase opcode of
RebaseOpcode::RebaseAndSkipping { times: 0xffffffff, skip: 0x4 }, the iterator will try to process the opcode in a single call toIterator::next(), pushing all the genearted symbols to an internal VecDeque, only to return them one at a time to the user on subsequent calls.This is bad because when
timesis an absurdly high number, the iterator will cause an OOM trying to push all the symbols to its VecDeque, and the user has no control on this behaviour.Change the implementation to be closer to what a proper generator would do, by remembering which instruction we were processing last time, restarting from where we left off, and decrementing
timesevery time we yield a value. This removes the need for allocations entirely.This does not fundamentally solve the issue, but at least now the iterator does not OOM when calling
next(), and the user can useIterator::take(limit)to avoid pulling an (almost) infinite stream of symbols.