Repository navigation
Coverity fixes - #4167
Coverity fixes#4167ethomson merged 6 commits into
Conversation
Sorry, something went wrong.
|
|
||
| git_buf_grow(&buf, alloc_len); | ||
| if (git_buf_grow(&buf, alloc_len) < 0) | ||
| return -1; |
There was a problem hiding this comment.
It looks like we're not doing any error checking on buf in the rest of this function. We should probably check that we didn't OOM while trying to work with this buffer before return.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed, thanks!
Sorry, something went wrong.
The OpenSSL library may require multiple locks to work correctly, where it is the caller's responsibility to initialize and release the locks. While we correctly initialized up to `n` locks, as determined by `CRYPTO_num_locks`, we were repeatedly freeing the same mutex in our shutdown procedure. Fix the issue by freeing locks at the correct index.
| diff->base.free_fn = diff_parsed_free; | ||
|
|
||
| git_diff_init_options(&diff->base.opts, GIT_DIFF_OPTIONS_VERSION); | ||
| if (git_diff_init_options(&diff->base.opts, GIT_DIFF_OPTIONS_VERSION) < 0) { |
There was a problem hiding this comment.
I was surprised to find that the git_*_init_options functions return an int, since none of them can fail. I guess it's nice future proofing. Anyway, if this shuts coverity up then we should take it.
Sorry, something went wrong.
There was a problem hiding this comment.
I was surprised by this, as well. And actually it can fail if somehow compiling against two versions of libgit2. Unlikely, but possible.
Sorry, something went wrong.
| git_buf_printf(&buf, "%s.", base_name); | ||
| if (git_buf_grow(&buf, alloc_len) < 0 | ||
| || git_buf_printf(&buf, "%s.", base_name) < 0) | ||
| goto end_parse; |
There was a problem hiding this comment.
I was thinking that we didn't even need to test this if we have the git_buf_oom test at the bottom. (Since the buffer will hold on to their OOM failure.) This lets us fail earlier, which could be nice. I don't have a strong preference, but I would suggest that you change the formatting so that the || is at the end of the first line, not starting the second.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm a bit torn, as well. Anyway, I think failing early here is a nice to have and it shuts up Coverity without any real detriment.
Huh. Where did I take up that "||" should start at newline... dunno. Will fix all occasions, thanks.
Sorry, something went wrong.
| git_object_lookup((git_object**)&porigin->blob, blame->repository, | ||
| git_blob_id(origin->blob), GIT_OBJ_BLOB); | ||
| if (!porigin->blob | ||
| && git_object_lookup((git_object**)&porigin->blob, blame->repository, |
There was a problem hiding this comment.
Can you pull the && up so that it's at the end of the first line, not starting the second?
Sorry, something went wrong.
|
|
||
| if (ctx->line_len > 0) { | ||
| if (parse_advance_expected_str(ctx, "\n") < 0 | ||
| || ctx->line_len > 0) { |
There was a problem hiding this comment.
Can you pull the || up onto the line above?
Sorry, something went wrong.
The function `pass_whole_blame` performs an object lookup but does not check if the lookup actually succeeds. Convert the function to return an error code and check for it in the calling function.
While parsing section headers, we use a buffer to store the actual section name. We do not check though if the buffer runs out of memory at any stage. Do so.
The `pack_entry_find_prefix` function receives a `git_rawobj` structure as argument. While the function first initializes the structure to a sensible state, Coverity is unable to correctly detect this, resulting in a warning. Fix this warning by initializing the object to all-zeroes before passing it to the function.
While parsing patch header lines, we iterate over each line and check if the line has trailing garbage. What we do not check though is that the line is actually a line ending with a trailing newline. Fix this by checking the return code of `parse_advance_expected_str`.
|
Thanks for your review, fixed all! |
Sorry, something went wrong.
|
Thanks! |
Sorry, something went wrong.
Some Coverity fixes. The most important one is the openssl_stream commit, which fixes an actual use after free/segfault when we shut down OpenSSL's mutexes.