buffer: fix writes into out-of-memory buffers #5232
Merged
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
|
/rebuild |
|
Sorry @ethomson, an error occurred while trying to requeue the build. |
|
Apparently you can't rebuild until the build finishes completely. I cancelled & requeued manually. |
|
Thanks! I tried to read the macOS log that was failing, but it really is completely unreadable due to spam caused by leak checks. :/ |
pks-t
force-pushed
the
pks/buffer-ensure-size-oom
branch
from
271f2c6
to
80be48e
Compare
|
Oops, there was a memory leak in core::buffer::avoid_printing_into_oom_buffer. |
ethomson
reviewed
Sep 20, 2019
tests/core/buffer.c
Outdated
| git_buf buf = GIT_BUF_INIT; | ||
|
|
||
| cl_git_pass(git_buf_puts(&buf, "foobar")); | ||
| cl_git_fail(git_buf_try_grow(&buf, SIZE_MAX, true)); |
There was a problem hiding this comment.
It looks like macOS isn't behaving the way you want. I think that you're relying on allocation semantics here and that you may need to substitute an intentionally failing allocator here that asserts that it ends up with SIZE_MAX and fails appropriately.
There was a problem hiding this comment.
Yeah, I expected this to happen but hoped it wouldn't, so I just gave it a try. I've fixed the tests to not rely on allocation semantics
pks-t
force-pushed
the
pks/buffer-ensure-size-oom
branch
2 times, most recently
from
df3f334
to
f3c9b51
Compare
If growing a buffer fails, we set its pointer to the static `git_buf__oom` structure. While we correctly free the old pointer if `git__malloc` returned an error, we do not free it if there was an integer overflow while calculating the new allocation size. Fix this issue by freeing the pointer to plug the memory leak.
When growing buffers, we repeatedly multiply the currently allocated
number of bytes by 1.5 until it exceeds the requested number of bytes.
This has two major problems:
1. If the current number of bytes is tiny and one wishes to resize
to a comparatively huge number of bytes, then we may need to loop
thousands of times.
2. If resizing to a value close to `SIZE_MAX` (which would fail
anyway), then we probably hit an infinite loop as multiplying the
current amount of bytes will repeatedly result in integer
overflows.
When reallocating buffers, one typically chooses values close to 1.5 to
enable re-use of resulting memory holes in later reallocations. But
because of this, it really only makes sense to use a factor of 1.5
_once_, but not looping until we finally are able to fit it. Thus, we
can completely avoid the loop and just opt for the much simpler
algorithm of multiplying with 1.5 once and, if the result doesn't fit,
just use the target size. This avoids both problems of looping
extensively and hitting overflows.
This commit also adds a test that would've previously resulted in an
infinite loop.
Before printing into a `git_buf` structure, we always call `ENSURE_SIZE` first. This macro will reallocate the buffer as-needed depending on whether the current amount of allocated bytes is sufficient or not. If `asize` is big enough, then it will just do nothing, otherwise it will call out to `git_buf_try_grow`. But in fact, it is insufficient to only check `asize`. When we fail to allocate any more bytes e.g. via `git_buf_try_grow`, then we set the buffer's pointer to `git_buf__oom`. Note that we touch neither `asize` nor `size`. So if we just check `asize > targetsize`, then we will happily let the caller of `ENSURE_SIZE` proceed with an out-of-memory buffer. As a result, we will print all bytes into the out-of-memory buffer instead, resulting in an out-of-bounds write. Fix the issue by having `ENSURE_SIZE` verify that the buffer is not marked as OOM. Add a test to verify that we're not writing into the OOM buffer.
pks-t
force-pushed
the
pks/buffer-ensure-size-oom
branch
from
f3c9b51
to
174b7a3
Compare
|
Finally green with another memory leak fix in the OOM case. |
ethomson
approved these changes
Sep 21, 2019
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.
This fixes two issues with
git_buf:SIZE_MAX. While obviously a bogus scenario, I still think the fix is worthwhile.git_buf__oombuffer.Found while investigating #5199 once again. This is probably not the cause for the observed crashes, though, as the stack traces point somewhere else.