Memory allocation audit#5200
Merged
Merged
Conversation
add2afe to
2a9f5b9
Compare
ethomson
reviewed
Aug 11, 2019
| GIT_UNUSED(hints); | ||
|
|
||
| if ((ainfo = malloc(sizeof(struct addrinfo))) == NULL) | ||
| if ((ainfo = git__malloc(sizeof(struct addrinfo))) == NULL) |
Member
There was a problem hiding this comment.
we'll need to p_free this in cleanup (line 35)
ethomson
reviewed
Aug 11, 2019
|
|
||
| for (p = 1; ainfo->ai_hostent->h_addr_list[p] != NULL; p++) { | ||
| if (!(ai->ai_next = malloc(sizeof(struct addrinfo)))) { | ||
| if (!(ai->ai_next = git__malloc(sizeof(struct addrinfo)))) { |
Member
There was a problem hiding this comment.
We'll need to p_free this in p_freeaddrinfo
ethomson
reviewed
Aug 11, 2019
| } | ||
|
|
||
| out->data = malloc(len); | ||
| out->data = git__malloc(len); |
Member
There was a problem hiding this comment.
Need to p_free this in p_unmap
The code in "blame_git.c" was mostly imported from git.git with only minor changes. One of these changes was to use our own allocators instead of git's `xmalloc`, but there's a subtle difference: `xmalloc` would abort the program if unable to allocate any memory, bit `git__malloc` doesn't. As we didn't check for memory allocation errors in some places, we might inadvertently dereference a `NULL` pointer in out-of-memory situations. Convert multiple functions to return proper error codes and add calls to `GIT_ERROR_CHECK_ALLOC` to fix this.
The function `git_commit_list_insert` dynamically allocates memory and may thus fail to insert a given commit, but we didn't check for that in several places in "merge.c". Convert surrounding functions to return error codes and check whether `git_commit_list_insert` was successful, returning an error if not.
When adding OIDs to the indexer's map of yet-to-be-seen OIDs to verify that packfiles are complete, we do so by first allocating a new OID and then calling `git_oidmap_set` on it. There was no check for memory allocation errors in place, though, leading to possible segfaults due to trying to copy data to a `NULL` pointer. Verify the result of `git__malloc` with `GIT_ERROR_CHECK_ALLOC` to fix the issue.
In "posix.c" there are multiple callsites which execute `malloc` instead of `git__malloc`. Thus, users of library are not able to track these allocations with a custom allocator. Convert these call sites to use `git__malloc` instead.
The "trailer.c" code has been copied mostly verbatim from git.git with minor adjustments, only. As git.git's `xmalloc` function, which aborts on memory allocation errors, has been swapped out for `git_malloc`, which doesn't abort, we may inadvertently access `NULL` pointers. Add checks to fix this.
When allocating a chunk that is used to write to HTTP streams, we do not check for memory allocation errors. This may lead us to write to a `NULL` pointer and thus cause a segfault. Fix this by adding a call to `GIT_ERROR_CHECK_ALLOC`.
The xdiff code contains multiple call sites where the results of `xdl_malloc` are not being checked for memory allocation errors. Add checks to fix possible segfaults due to `NULL` pointer accesses.
Our hand-rolled fallback sorting function `git__insertsort_r` does an in-place sort of the given array. As elements may not necessarily be pointers, it needs a way of swapping two values of arbitrary size, which is currently implemented by allocating a temporary buffer of the element's size. This is problematic, though, as the emulated `qsort` interface doesn't provide any return values and thus cannot signal an error if allocation of that temporary buffer has failed. Convert the function to swap via a temporary buffer allocated on the stack. Like this, it can `memcpy` contents of both elements in small batches without requiring a heap allocation. The buffer size has been chosen such that in most cases, a single iteration of copying will suffice. Most importantly, it can fully contain `git_oid` structures and pointers. Add a bunch of tests for the `git__qsort_r` interface to verify nothing breaks. Furthermore, this removes the declaration of `git__insertsort_r` and makes it static as it is not used anywhere else.
2a9f5b9 to
8cbef12
Compare
Member
Author
|
Thanks, good catch @ethomson! Fixed the missing free conversions |
This file contains hidden or 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
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.
Various fixes to potential memory allocation errors which were previously unchecked. Triggered by #5199 and #5164, even though I've got no idea whether it will fix these reported issues.