coverity: fix leaks and error paths#2163
Open
dscho wants to merge 13 commits into
Open
Conversation
Pointed out by Coverity. While at it, reduce near-duplicate clean-up code at the end of the function. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
write_one_object() opens a file at line 186 and jumps to the errout label on failure. The errout cleanup unconditionally calls close(fd), but when open() itself failed, fd is -1. Calling close(-1) is harmless on most platforms (returns EBADF) but is undefined behavior per POSIX and can confuse fd tracking in sanitizer builds. Guard the close with fd >= 0. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When the `git-remote-https` command fails, we do not want to leak `child_out`. Pointed out by Coverity. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When start_command() fails to set up a pipe partway through, it
rolls back by closing the pipe ends it has already opened. For
descriptors supplied by the caller rather than allocated locally,
that rollback tested `if (cmd->in)` / `if (cmd->out)` before calling
close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is
non-zero and so passes the test, meaning a caller that sets
cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up
triggering close(-1) on the error path.
The stdin-pipe failure branch a few lines above already uses the
right idiom, `if (cmd->out > 0)`, which rejects both the -1 sentinel
and 0 (the parent's own standard streams). Apply it to the three
remaining rollback sites.
Reported by Coverity as CID 1049722 ("Argument cannot be negative").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In 4fc970c (diff --cc: fix display of symlink conflicts during a merge., 2007-02-25) a conditional block was introduced in `run_diff_files()` that skips the rest of the loop iteration and advances directly to the next iteration. However, it missed that there was a similar conditional block that was last touched in b4b1550 (Don't instantiate structures with FAMs., 2006-06-18) and which demonstrated that the `dpath` structure needed to be released. Let's fix this. Pointed out by Coverity. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When bloom_filter_check() indicates that a commit does not touch any of the tracked paths, line_log_process_ranges_arbitrary_commit() propagates the current ranges to the parent by calling line_log_data_copy() and passing the copy to add_line_range(). However, add_line_range() always makes its own copy internally (via line_log_data_copy or line_log_data_merge), so the caller's copy is never freed and leaks every time this path is taken. Pass range directly to add_line_range() instead of making a redundant intermediate copy. The callee's internal copy handles ownership correctly. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When read_one_dir() encounters a parse error while reading the untracked cache from disk, it returns -1 immediately. Two allocations made earlier in the function can leak on these early-return paths: ud.untracked (allocated at line 3846 when untracked_nr > 0) and ud.dirs (allocated at line 3851). Free both before returning on the two error paths between these allocations and the point where they are transferred into the final xmalloc'd struct at line 3857. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
get_superproject_working_tree() allocates cwd via xgetcwd() at the top of the function, but two early-return paths (when not inside a work tree, and when strbuf_realpath for "../" fails) return 0 without freeing it. Redirect these early returns through a cleanup label that frees cwd before returning. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In the "add" subcommand, when run_command() fails while creating a new branch (line 948), the function returns -1 immediately without freeing the allocations made earlier: path (from prefix_filename at line 858), opt_track, branch_to_free, and new_branch_to_free. Redirect the error return through the existing cleanup block at the end of the function so all four allocations are properly freed. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When uploading messages via libcurl, curl_append_msgs_to_imap() accumulates each one in a strbuf that grows across loop iterations but is never released before the function returns. Release it alongside the existing libcurl cleanup. Reported by Coverity as CID 1671507 ("Resource leak"). Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator and then calls reftable_buf_add() to populate its oid buffer. On success ownership is transferred to the output iterator, but if reftable_buf_add() fails, the goto-out cleanup only frees the table iterator and walks away from both the filter allocation and the oid buffer that reftable_buf_add() may have grown. Release filter->oid and free filter alongside the existing table iterator cleanup. Reported by Coverity as CID 1671512 ("Resource leak"). Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
`fsmonitor_run_daemon()` allocates `state.current_token_data` before any subordinate setup step that may fail (alias resolution, listener/health constructors, asynchronous IPC server init). On the successful path the listener thread takes ownership and clears the field during its teardown, so the `done:` cleanup block sees a NULL pointer. On every early-error path, however, control jumps straight to `done:` with the freshly allocated token data still referenced, and it is never freed, as Coverity flagged. Free it at the top of `done:` and clear the pointer. The success path is a no-op (the pointer is already NULL there); the error paths now drop the otherwise-leaked allocation. `fsmonitor_free_token_data()` is NULL-safe and asserts `client_ref_count == 0`, which holds trivially here because the IPC server has not yet begun accepting clients when these failures occur. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
After "mingw: kill child processes in a gentler way", the ownership of the HANDLE passed to exit_process() and terminate_process_tree() is inconsistent. terminate_process_tree() always closes the handle; exit_process() closes it on success and on the terminate-tree fallback, but leaks it on the early return where GetExitCodeProcess() fails or reports the process is no longer STILL_ACTIVE. mingw_kill() compensated by closing the handle on its own error path, which is a double-close on every error path that does not hit that one leaky branch -- the callee has already closed the handle by then. Coverity flagged the resulting use-after-free as CID 1437238. Pin down the invariant that exit_process() and terminate_process_tree() own the handle from the call onward and close it on every return path; with that, the bogus close in mingw_kill() goes away. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2163.git.1782889472.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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
1 participant
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.
I wanted to whittle down the many issues reported by Coverity in the Git for Windows project. Turns out: The vast majority of the issues are false positives. Most of the remaining issues are in core Git proper.
This effort was forced on pause while Coverity was down from May 16 to June 22).
Here is a first batch of fixes for those issues.