realloc: Copy old_size
number of bytes (not new_size
) into new allocation
#70
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.
When reallocating, if we allocate new space, we need to copy the old
allocation's bytes into the new space. There are
old_size
number of bytes inthe old allocation, but we were accidentally copying
new_size
number of bytes,which could lead to copying bytes into the realloc'd space from past the chunk
that we're bump allocating out of, from unknown memory.
If an attacker can cause
realloc
s, and can read therealoc
ed data back, thiscould allow them to read things from other regions of memory that they shouldn't
be able to. For example, if some crypto keys happened to live in memory right
after a chunk we were bump allocating out of, this could allow the attacker to
read the crypto keys.
Beyond just fixing the bug and adding a regression test, I've also taken two
additional steps:
While we were already running the testsuite under
valgrind
in CI, becausevalgrind
exits with the same code that the program did, if there are invalidreads/writes that happen not to trigger a segfault, the program can still exit
OK and we will be none the wiser. I've enabled the
--error-exitcode=1
flag forvalgrind
in CI so that tests eagerly fail in these scenarios.I've written a quickcheck test to exercise
realloc
. Without the bug fix inthis patch, this quickcheck immediately triggers invalid reads when run under
valgrind
. We didn't previously have quickchecks that exercisedrealloc
beacuse
realloc
isn't publicly exposed directly, and instead can only beindirectly called. This new quickcheck test exercises
realloc
viabumpalo::collections::Vec::resize
andbumpalo::collections::Vec::shrink_to_fit
calls.Fixes #69