coverity: fix leaks and error paths#2163
Open
dscho wants to merge 12 commits into
Open
Conversation
Member
Author
|
/submit |
|
Submitted as pull.2163.git.1782889472.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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>
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>
Two of `read_one_dir()`'s parse-error early returns leak ud.untracked and ud.dirs. Plug them. The other early returns in the same function are fine: they occur after the `xmalloc()`+`memcpy()` that copies ud into `*untracked_`, at which point ownership is transferred to the caller. `read_untracked_extension()` then releases everything via `free_untracked_cache()` on failure. 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>
dscho
force-pushed
the
coverity-fixes-leaks-and-error-paths
branch
from
23ab986 to
a5a6c27
Compare
Member
Author
|
/submit |
|
Submitted as pull.2163.v2.git.1783239870.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@f621efc. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -202,7 +202,8 @@ static int write_one_object(struct odb_source_loose *loose, | |||
| return 0; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:20AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 47b7f5ec38..2c6db45245 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -202,7 +202,8 @@ static int write_one_object(struct odb_source_loose *loose,
> return 0;
> errout:
> error_errno(_("failed to write loose object index %s"), path.buf);
> - close(fd);
> + if (fd >= 0)
> + close(fd);
> rollback_lock_file(&lock);
> strbuf_release(&buf);
> strbuf_release(&path);
Makes sense. At the time we hit the first `goto errout` we have already
assigned `fd = open(...)`, so we know it should be either negative or a
positive file descriptor.
There's also a second call to `close(fd)`, but if that call is
successful then we would not use the `errout` path. If it fails we may
try to close the file descriptor a second time, but that's probably a
non-issue.
Patrick
|
User |
| @@ -706,7 +706,7 @@ int start_command(struct child_process *cmd) | |||
| failed_errno = errno; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:22AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/run-command.c b/run-command.c
> index e70a8a387b..ce84db8782 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
> failed_errno = errno;
> if (need_in)
> close_pair(fdin);
> - else if (cmd->in)
> + else if (cmd->in > 0)
> close(cmd->in);
> str = "standard output";
> goto fail_pipe;
> @@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
> failed_errno = errno;
> if (need_in)
> close_pair(fdin);
> - else if (cmd->in)
> + else if (cmd->in > 0)
> close(cmd->in);
> if (need_out)
> close_pair(fdout);
> - else if (cmd->out)
> + else if (cmd->out > 0)
> close(cmd->out);
> str = "standard error";
> fail_pipe:
Right. There's a fourth site that does `close(cmd->out)`, but that site
already guards with `if (cmd->out > 0)`.
Patrick
|
|
||
| if (revs->diffopt.prefix && | ||
| strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length)) | ||
| continue; |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:23AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..7ba839b4a8 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -152,7 +152,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> continue;
>
> if (ce_stage(ce)) {
> - struct combine_diff_path *dpath;
> + struct combine_diff_path *dpath = NULL;
> struct diff_filepair *pair;
> unsigned int wt_mode = 0;
> int num_compare_stages = 0;
> @@ -164,6 +164,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> else {
> if (changed < 0) {
> perror(ce->name);
> + free(dpath);
> continue;
> }
> wt_mode = 0;
Huh. There is no assignment between the variable declaration and this
call to `continue`, so how could this ever plug a memory leak? None of
the other paths seem to leak the variable, either.
PatrickThere was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Patrick,
On Wed, 1 Jul 2026, Patrick Steinhardt wrote:
> On Wed, Jul 01, 2026 at 07:04:23AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/diff-lib.c b/diff-lib.c
> > index ae91027a02..7ba839b4a8 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > @@ -152,7 +152,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> > continue;
> >
> > if (ce_stage(ce)) {
> > - struct combine_diff_path *dpath;
> > + struct combine_diff_path *dpath = NULL;
> > struct diff_filepair *pair;
> > unsigned int wt_mode = 0;
> > int num_compare_stages = 0;
> > @@ -164,6 +164,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> > else {
> > if (changed < 0) {
> > perror(ce->name);
> > + free(dpath);
> > continue;
> > }
> > wt_mode = 0;
>
> Huh. There is no assignment between the variable declaration and this
> call to `continue`, so how could this ever plug a memory leak? None of
> the other paths seem to leak the variable, either.
You are right; the patch as posted plugs nothing.
The reason it looks pointless is that the leak it was written against was
fixed independently in the meantime by 949bb8f74f4a (run_diff_files():
delay allocation of combine_diff_path, 2025-01-09), which moved the `dpath
= xmalloc(...)` to after the `check_removed()` call. Before that
reordering, the two `continue` statements did leak the just-allocated
`dpath` (originally introduced by 4fc970c43884, 2007-02-25).
I had missed that when picking back up the work on addressing Coverity
reports, sorry! I will drop this patch from v2.
Ciao,
Johannes
| @@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf) | |||
| * We might have a superproject, but it is harder | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:26AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/submodule.c b/submodule.c
> index fd91201a92..8ddeebd8af 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> * We might have a superproject, but it is harder
> * to determine.
> */
> - return 0;
> + goto out;
>
> if (!strbuf_realpath(&one_up, "../", 0))
> - return 0;
> + goto out;
>
> subpath = relative_path(cwd, one_up.buf, &sb);
> strbuf_release(&one_up);
> @@ -2693,6 +2693,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> die(_("ls-tree returned unexpected return code %d"), code);
>
> return ret;
> +
> +out:
> + free(cwd);
> + return 0;
> }
Okay. This is fine, but it feels a bit fragile as we also have a call to
`free(cwd)` a bit further up. So if somebody were to add a `goto out`
after that call we'd have a double free. Makes me wonder whether we want
to have a single exit path for the complete function and then drop the
other call to free(3p).
PatrickThere was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Patrick,
On Wed, 1 Jul 2026, Patrick Steinhardt wrote:
> On Wed, Jul 01, 2026 at 07:04:26AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/submodule.c b/submodule.c
> > index fd91201a92..8ddeebd8af 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> > * We might have a superproject, but it is harder
> > * to determine.
> > */
> > - return 0;
> > + goto out;
> >
> > if (!strbuf_realpath(&one_up, "../", 0))
> > - return 0;
> > + goto out;
> >
> > subpath = relative_path(cwd, one_up.buf, &sb);
> > strbuf_release(&one_up);
> > @@ -2693,6 +2693,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> > die(_("ls-tree returned unexpected return code %d"), code);
> >
> > return ret;
> > +
> > +out:
> > + free(cwd);
> > + return 0;
> > }
>
> Okay. This is fine, but it feels a bit fragile as we also have a call to
> `free(cwd)` a bit further up. So if somebody were to add a `goto out`
> after that call we'd have a double free. Makes me wonder whether we want
> to have a single exit path for the complete function and then drop the
> other call to free(3p).
Agreed. In v2 the function has a single exit path: all late returns
fall through to the `out:` label, which additionally releases `sb`
and `one_up`.
A side effect worth noting is that consolidation also closes a latent
leak the original had on the `strbuf_realpath(&one_up, "../", 0)`
failure path. `strbuf_realpath_1()` calls `strbuf_reset(resolved)` on
error, which does not free the backing buffer, so `one_up` could
carry a residual allocation that the previous shape never released.
Ciao,
Johannes
| @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_, | |||
| ALLOC_ARRAY(ud.untracked, ud.untracked_nr); | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/dir.c b/dir.c
> index 32430090dc..23335b9f7a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
> ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
>
> ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
> - if (data > end)
> + if (data > end) {
> + free(ud.untracked);
> return -1;
> + }
> ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
>
> eos = memchr(data, '\0', end - data);
> - if (!eos || eos == end)
> + if (!eos || eos == end) {
> + free(ud.untracked);
> + free(ud.dirs);
> return -1;
> + }
>
> *untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
> memcpy(untracked, &ud, sizeof(ud));
Hm. Here we assign ownership to the caller, but this still feels quite
off to me as we also have two more early returns after this point that
seem to leak memory. Do the callers make sure to always free the data?
PatrickThere was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Patrick,
On Wed, 1 Jul 2026, Patrick Steinhardt wrote:
> On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/dir.c b/dir.c
> > index 32430090dc..23335b9f7a 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
> > ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
> >
> > ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
> > - if (data > end)
> > + if (data > end) {
> > + free(ud.untracked);
> > return -1;
> > + }
> > ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
> >
> > eos = memchr(data, '\0', end - data);
> > - if (!eos || eos == end)
> > + if (!eos || eos == end) {
> > + free(ud.untracked);
> > + free(ud.dirs);
> > return -1;
> > + }
> >
> > *untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
> > memcpy(untracked, &ud, sizeof(ud));
>
> Hm. Here we assign ownership to the caller, but this still feels quite
> off to me as we also have two more early returns after this point that
> seem to leak memory. Do the callers make sure to always free the data?
Ownership transfers to the caller on the `xmalloc`/`memcpy` line: the
`memcpy` copies the `ud.untracked` and `ud.dirs` pointers into the freshly
xmalloc'd struct that becomes `*untracked_`. From there, any subsequent
failure in the caller reaches `free_untracked_cache()` and then
`free_untracked()`, which releases both arrays. So the two further early
returns are correct as-are.
I will fold that reasoning into the v2 commit message so a future
reader does not have to re-derive it.
Incidentally, and orthogonal to Coverity's leak report: on those same
failure paths, individual slots of `->dirs` and `->untracked` remain
uninitialised, so `free_untracked()` walks garbage pointers before it
ever reaches the two `free()` calls above. That is a separate
crash-on-cleanup bug and I would prefer to address it in a follow-up
rather than widen the scope of this series.
Ciao,
Johannes
| @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_ | |||
| { | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:19AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 0b626c1b85..47b7f5ec38 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> {
> struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
> FILE *fp;
> + int ret = -1;
>
> if (!loose->map)
> loose_object_map_init(&loose->map);
> @@ -98,13 +99,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> insert_loose_map(loose, &oid, &compat_oid);
> }
>
> - strbuf_release(&buf);
> - strbuf_release(&path);
> - return errno ? -1 : 0;
> + ret = 0;
> err:
> + fclose(fp);
> strbuf_release(&buf);
> strbuf_release(&path);
> - return -1;
> + return ret;
> }
Makes sense. There's no `goto err` before we assign `fp`, and when the
call to `fopen()` fails we return via a different path. So the added
call to `fclose(fp)` is fine.
Patrick
| @@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit | |||
|
|
|||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Wed, Jul 01, 2026 at 07:04:24AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: 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.
Heh, I just posted the identical patch (in my case found by running the
test suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1).
So yeah, looks good to me. :)
-Peff
|
User |
| @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_ | |||
| { | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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>
> ---
> loose.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/loose.c b/loose.c
> index 0b626c1b85..47b7f5ec38 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> {
> struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
> FILE *fp;
> + int ret = -1;
>
> if (!loose->map)
> loose_object_map_init(&loose->map);
> @@ -98,13 +99,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> insert_loose_map(loose, &oid, &compat_oid);
> }
>
> - strbuf_release(&buf);
> - strbuf_release(&path);
> - return errno ? -1 : 0;
Wow, this is bad bad bad. We do not even know what is in errno as
we are supposed to have jumped to out-of-line err label in all error
cases.
> + ret = 0;
Or we can do
ret = ferror(fp) ? -1 : 0;
if we want to be sure that we have caught all the errors.
> err:
> + fclose(fp);
> strbuf_release(&buf);
> strbuf_release(&path);
> - return -1;
> + return ret;
> }
>
> int repo_read_loose_object_map(struct repository *repo)There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Junio,
On Wed, 1 Jul 2026, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > + ret = 0;
>
> Or we can do
>
> ret = ferror(fp) ? -1 : 0;
>
> if we want to be sure that we have caught all the errors.
Agreed; that is what v2 will use.
To corroborate the diagnosis: `strbuf_getline_lf()` ultimately calls
`getdelim()`, which returns -1 on both EOF and I/O error, so `ferror(fp)`
on the underlying stream is the only reliable way to distinguish the two.
That also makes the `errno = 0;` I had added at the top of the loop dead,
so it goes away in v2.
Ciao,
Johannes
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
I read through the series and did not see anything jumping at me as
wrong. Looking good. Will queue.
Thanks.
>
> This effort was forced on pause while Coverity was down from May 16
> [https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to
> June 22
> [https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).
>
> Here is a first batch of fixes for those issues.
>
> Johannes Schindelin (13):
> load_one_loose_object_map(): fix resource leak
> loose: avoid closing invalid fd on error path
> download_https_uri_to_file(): do not leak fd upon failure
> run-command: avoid close(-1) in start_command() error paths
> run_diff_files: avoid memory leak
> line-log: avoid redundant copy that leaks in process_ranges
> dir: free allocations on parse-error paths in read_one_dir()
> submodule: fix cwd leak in get_superproject_working_tree()
> worktree: fix resource leaks when branch creation fails
> imap-send: avoid leaking the IMAP upload buffer
> reftable/table: release filter on error path
> fsmonitor: plug token-data leak on early daemon-startup failures
> mingw: make exit_process() own the process handle on all paths
>
> builtin/fsmonitor--daemon.c | 2 ++
> builtin/worktree.c | 7 +++++--
> bundle-uri.c | 2 +-
> compat/mingw.c | 4 +---
> compat/win32/exit-process.h | 1 +
> diff-lib.c | 3 ++-
> dir.c | 9 +++++++--
> imap-send.c | 1 +
> line-log.c | 3 +--
> loose.c | 11 ++++++-----
> reftable/table.c | 4 ++++
> run-command.c | 6 +++---
> submodule.c | 8 ++++++--
> 13 files changed, 40 insertions(+), 21 deletions(-)
>
>
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2163%2Fdscho%2Fcoverity-fixes-leaks-and-error-paths-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2163/dscho/coverity-fixes-leaks-and-error-paths-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2163 |
|
There was a status update in the "New Topics" section about the branch A collection of fixes for various resource leaks, invalid file descriptor closures, and process handle ownership issues flagged by Coverity. Waiting for response(s) to review comment(s). cf. <akTIMM6qLfDNdg-a@pks.im> source: <pull.2163.git.1782889472.gitgitgadget@gmail.com> |
| @@ -2627,13 +2627,12 @@ int get_superproject_working_tree(struct strbuf *buf) | |||
| * We might have a superproject, but it is harder | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Sun, Jul 05, 2026 at 08:24:24AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/submodule.c b/submodule.c
> index fd91201a92..92dfb0fc2d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2627,13 +2627,12 @@ int get_superproject_working_tree(struct strbuf *buf)
> * We might have a superproject, but it is harder
> * to determine.
> */
> - return 0;
> + goto out;
>
> if (!strbuf_realpath(&one_up, "../", 0))
> - return 0;
> + goto out;
>
> subpath = relative_path(cwd, one_up.buf, &sb);
> - strbuf_release(&one_up);
>
> prepare_submodule_repo_env(&cp.env);
> strvec_pop(&cp.env);
Right. `ret` is already zero-initialized at the beginning of the
function, so it's fine to just `goto out` here.
> @@ -2678,20 +2677,22 @@ int get_superproject_working_tree(struct strbuf *buf)
> ret = 1;
> free(super_wt);
> }
> - free(cwd);
> - strbuf_release(&sb);
>
> code = finish_command(&cp);
>
> if (code == 128)
> /* '../' is not a git repository */
> - return 0;
> - if (code == 0 && len == 0)
> + ret = 0;
> + else if (code == 0 && len == 0)
> /* There is an unrelated git repository at '../' */
> - return 0;
> - if (code)
> + ret = 0;
> + else if (code)
> die(_("ls-tree returned unexpected return code %d"), code);
The diff is a bit hard to read as we also convert this to use `else if`,
but overall the end result is easier to reason about.
> +out:
> + strbuf_release(&sb);
> + strbuf_release(&one_up);
> + free(cwd);
> return ret;
> }
All of these variables are always initialized, so this change looks good
to me.
Thanks!
Patrick
| @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_ | |||
| { | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Sun, Jul 05, 2026 at 08:24:18AM +0000, Johannes Schindelin via GitGitGadget wrote:
> @@ -98,13 +98,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> insert_loose_map(loose, &oid, &compat_oid);
> }
>
> - strbuf_release(&buf);
> - strbuf_release(&path);
> - return errno ? -1 : 0;
> + ret = ferror(fp) ? -1 : 0;
> err:
> + fclose(fp);
> strbuf_release(&buf);
> strbuf_release(&path);
> - return -1;
> + return ret;
Nit: it might've made sense to explain the switch to ferror(3p) in the
commit message, but that alone isn't worth a reroll.
Patrick
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Sun, Jul 05, 2026 at 08:24:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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
> [https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to
> June 22
> [https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).
>
> Here is a first batch of fixes for those issues.
>
> Changes since v1:
>
> * Edited the commit messages to put function names in backticks, and
> reflowed the messages afterwards.
> * Took Junio's suggestion to avoid (ab-)using errno to determine the return
> value of load_one_loose_object_map().
> * Dropped the obsolete patch "run_diff_files: avoid memory leak".
> * Rewrote the commit message of "dir: free allocations on parse-error paths
> in read_one_dir()" to clarify ownership of the allocated untracked/dirs
> buffers.
> * Changed "submodule: fix cwd leak in get_superproject_working_tree()" to
> reduce the cognitive load on the reader (i.e. to make it a lot easier to
> reason about the correctness of the patch).
Thanks. The reflow of the commit messages made the range-diff somewhat
hard to read, but from all I could see the changes all make sense.
Patrick |
|
There was a status update in the "Cooking" section about the branch Various resource leaks, invalid file descriptor closures, and process handle ownership issues flagged by Coverity have been fixed. Needs review. source: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com> |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
> On Sun, Jul 05, 2026 at 08:24:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
>> 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
>> [https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to
>> June 22
>> [https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).
>>
>> Here is a first batch of fixes for those issues.
>>
>> Changes since v1:
>>
>> * Edited the commit messages to put function names in backticks, and
>> reflowed the messages afterwards.
>> * Took Junio's suggestion to avoid (ab-)using errno to determine the return
>> value of load_one_loose_object_map().
>> * Dropped the obsolete patch "run_diff_files: avoid memory leak".
>> * Rewrote the commit message of "dir: free allocations on parse-error paths
>> in read_one_dir()" to clarify ownership of the allocated untracked/dirs
>> buffers.
>> * Changed "submodule: fix cwd leak in get_superproject_working_tree()" to
>> reduce the cognitive load on the reader (i.e. to make it a lot easier to
>> reason about the correctness of the patch).
>
> Thanks. The reflow of the commit messages made the range-diff somewhat
> hard to read, but from all I could see the changes all make sense.
Yup, this round looks good to me, too. Thanks, both.
|
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.
Changes since v1:
errnoto determine the return value ofload_one_loose_object_map().read_one_dir()" to clarify ownership of the allocateduntracked/dirsbuffers.get_superproject_working_tree()" to reduce the cognitive load on the reader (i.e. to make it a lot easier to reason about the correctness of the patch).cc: Patrick Steinhardt ps@pks.im
cc: Jeff King peff@peff.net