-
Notifications
You must be signed in to change notification settings - Fork 192
coverity: fix unchecked returns #2179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e653255
0692704
9bf7e73
711671c
72a74c7
f0b1e13
0facb9e
2b0e4f3
7f2b963
9a91030
829cd82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -485,22 +485,29 @@ static int bisect_next_check(const struct bisect_terms *terms, | |
| return decide_next(terms, current_term, !state.nr_good, !state.nr_bad); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> There is one slight complication here: One caller _needs_ the return
> value to indicate an error when the `BISECT_TERMS` file is absent, all
> the other call sites are totally okay with a "missing" `BISECT_TERMS`
> file. To address that, extend the function signature of `get_terms()` to
> indicate which behavior the caller wants.
> -static int get_terms(struct bisect_terms *terms)
> +static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
> {
> struct strbuf str = STRBUF_INIT;
> FILE *fp = NULL;
> @@ -493,7 +493,7 @@ static int get_terms(struct bisect_terms *terms)
>
> fp = fopen(git_path_bisect_terms(), "r");
> if (!fp) {
> - res = -1;
> + res = file_missing_is_ok ? 0 : -1;
> goto finish;
> }
Hmph. So, depending on the caller, a missing file error may have to
be treated as OK or as an error, while all other kinds of anomalies
are treated by all callers as errors.
As all the existing callsites of this function need to be adjusted
for this change anyway, I would have thought a more typical way to
handle a situation like this would be to define different error
codes for this function and have the callers deal with them. But it
seems that almost all callers, except for one, pass "missing is OK."
So, instead of adjusting the majority of callers with something like:
- if (get_terms(...))
+ if (get_terms(...) == BISECT_TERMS_ERROR)
oops we got an error
and keeping only the single oddball caller to barf on any non-zero
return,
- if (get_terms(...))
+ switch (get_terms(...)) {
+ case BISECT_TERMS_ERROR:
oops we got an error
+ break;
+ case BISECT_TERMS_MISSING_FILE:
+ deal with the missing file error
+ break;
+ default:
+ break; /* ok */
+ }
it may be simpler to change:
- if (get_terms(...))
+ if (get_terms(..., 1))
oops we got an error
for the majority of them. The one oddball caller then becomes:
- if (get_terms(...))
+ if (get_terms(..., 0))
oops we got an error
to treat a missing file as an error as well.
I guess I can buy that.
If get_terms() were a public function that had many more callers,
my preference would probably be very different. But this is local
to a single file, so the meaning of the mysterious 0/1 parameter
will quickly become evident to those who have to work with this
part of the system anyway.
Thanks. |
||
| } | ||
|
|
||
| static int get_terms(struct bisect_terms *terms) | ||
| static int get_terms(struct bisect_terms *terms, int file_missing_is_ok) | ||
| { | ||
| struct strbuf str = STRBUF_INIT; | ||
| FILE *fp = NULL; | ||
| int res = 0; | ||
|
|
||
| fp = fopen(git_path_bisect_terms(), "r"); | ||
| if (!fp) { | ||
| res = -1; | ||
| res = file_missing_is_ok ? 0 : -1; | ||
| goto finish; | ||
| } | ||
|
|
||
| free_terms(terms); | ||
| strbuf_getline_lf(&str, fp); | ||
| if (strbuf_getline_lf(&str, fp) == EOF) { | ||
| res = -1; | ||
| goto finish; | ||
| } | ||
| terms->term_bad = strbuf_detach(&str, NULL); | ||
| strbuf_getline_lf(&str, fp); | ||
| if (strbuf_getline_lf(&str, fp) == EOF) { | ||
| res = -1; | ||
| FREE_AND_NULL(terms->term_bad); | ||
| goto finish; | ||
| } | ||
| terms->term_good = strbuf_detach(&str, NULL); | ||
|
|
||
| finish: | ||
|
|
@@ -512,7 +519,7 @@ static int get_terms(struct bisect_terms *terms) | |
|
|
||
| static int bisect_terms(struct bisect_terms *terms, const char *option) | ||
| { | ||
| if (get_terms(terms)) | ||
| if (get_terms(terms, 0)) | ||
| return error(_("no terms defined")); | ||
|
|
||
| if (!option) { | ||
|
|
@@ -1050,7 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line) | |
| rev = word_end + strspn(word_end, " \t"); | ||
| *word_end = '\0'; /* NUL-terminate the word */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Jul 14, 2026 at 10:48:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Six callers of get_terms() silently discard its return value. When
> get_terms fails (missing or truncated BISECT_TERMS file), the term
> strings remain NULL or empty, causing confusing downstream
> behavior: commands like "bisect next" or "bisect run" proceed with
> empty term strings, producing nonsensical ref names (refs/bisect/
> with no suffix) and misleading error messages.
>
> Add checks at each call site so that a failed get_terms produces a
> clear "no terms defined" error, matching the pattern already used
> in bisect_terms() at line 512. The check tests the term pointers
> rather than the return value because some callers (bisect skip,
> legacy bad/good) call set_terms before get_terms, and the
> set_terms values should survive a get_terms failure.
Hm. Are there any callers that accept the case where either `term->bad`
or `term->good` are `NULL`? If not, should we maybe adapt the function
itself to return an error if so and then have all callers only ever
check for the return value of `get_term()` instead of also having to
check the result? That might also allow us to deduplicate the error
messages.
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Wed, 15 Jul 2026, Patrick Steinhardt wrote:
> On Tue, Jul 14, 2026 at 10:48:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Six callers of get_terms() silently discard its return value. When
> > get_terms fails (missing or truncated BISECT_TERMS file), the term
> > strings remain NULL or empty, causing confusing downstream
> > behavior: commands like "bisect next" or "bisect run" proceed with
> > empty term strings, producing nonsensical ref names (refs/bisect/
> > with no suffix) and misleading error messages.
> >
> > Add checks at each call site so that a failed get_terms produces a
> > clear "no terms defined" error, matching the pattern already used
> > in bisect_terms() at line 512. The check tests the term pointers
> > rather than the return value because some callers (bisect skip,
> > legacy bad/good) call set_terms before get_terms, and the
> > set_terms values should survive a get_terms failure.
>
> Hm. Are there any callers that accept the case where either `term->bad`
> or `term->good` are `NULL`?
As far as I can tell, no, the case where either `term->bad` or
`term->good` are `NULL` is not permissible.
> If not, should we maybe adapt the function itself to return an error if
> so and then have all callers only ever check for the return value of
> `get_term()` instead of also having to check the result? That might also
> allow us to deduplicate the error messages.
It's a good point that we should not look at `term->bad` and `term->good`,
but at the return value of `get_term()` instead. That's incidentally what
`bisect_terms()` does, and we should do the same here (including the same,
already-translated error message).
Thanks,
Johannes
|
||
|
|
||
| get_terms(terms); | ||
| if (get_terms(terms, 1)) | ||
| return error(_("no terms defined")); | ||
| if (check_and_set_terms(terms, p)) | ||
| return -1; | ||
|
|
||
|
|
@@ -1300,7 +1308,12 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv) | |
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Jul 14, 2026 at 10:48:44PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 15a2a30f89..801daf8c78 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -1308,6 +1308,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>
> fflush(stdout);
> saved_stdout = dup(1);
> + if (saved_stdout < 0) {
> + res = error_errno(_("could not duplicate stdout"));
> + close(temporary_stdout_fd);
> + break;
> + }
> dup2(temporary_stdout_fd, 1);
Shouldn't we also verify the return value of `dup2()` while at it?
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Wed, 15 Jul 2026, Patrick Steinhardt wrote:
> On Tue, Jul 14, 2026 at 10:48:44PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/builtin/bisect.c b/builtin/bisect.c
> > index 15a2a30f89..801daf8c78 100644
> > --- a/builtin/bisect.c
> > +++ b/builtin/bisect.c
> > @@ -1308,6 +1308,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
> >
> > fflush(stdout);
> > saved_stdout = dup(1);
> > + if (saved_stdout < 0) {
> > + res = error_errno(_("could not duplicate stdout"));
> > + close(temporary_stdout_fd);
> > + break;
> > + }
> > dup2(temporary_stdout_fd, 1);
>
> Shouldn't we also verify the return value of `dup2()` while at it?
True. I wonder why Coverity didn't complain... funny. I changed it to also
check the return value of `dup2()`.
Thank you for your review!
Johannes |
||
| fflush(stdout); | ||
| saved_stdout = dup(1); | ||
| dup2(temporary_stdout_fd, 1); | ||
| if (saved_stdout < 0 || | ||
| dup2(temporary_stdout_fd, 1) < 0) { | ||
| res = error_errno(_("could not duplicate stdout")); | ||
| close(temporary_stdout_fd); | ||
| break; | ||
| } | ||
|
|
||
| res = bisect_state(terms, 1, &new_state); | ||
|
|
||
|
|
@@ -1376,7 +1389,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref | |
| if (argc) | ||
| return error(_("'%s' requires 0 arguments"), | ||
| "git bisect next"); | ||
| get_terms(&terms); | ||
| if (get_terms(&terms, 1)) | ||
| return error(_("no terms defined")); | ||
| res = bisect_next(&terms, prefix); | ||
| free_terms(&terms); | ||
| return res; | ||
|
|
@@ -1410,7 +1424,8 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS | |
| struct bisect_terms terms = { 0 }; | ||
|
|
||
| set_terms(&terms, "bad", "good"); | ||
| get_terms(&terms); | ||
| if (get_terms(&terms, 1)) | ||
| return error(_("no terms defined")); | ||
| res = bisect_skip(&terms, argc, argv); | ||
| free_terms(&terms); | ||
| return res; | ||
|
|
@@ -1422,7 +1437,8 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix | |
| int res; | ||
| struct bisect_terms terms = { 0 }; | ||
|
|
||
| get_terms(&terms); | ||
| if (get_terms(&terms, 1)) | ||
| return error(_("no terms defined")); | ||
| res = bisect_visualize(&terms, argc, argv); | ||
| free_terms(&terms); | ||
| return res; | ||
|
|
@@ -1436,7 +1452,8 @@ static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSE | |
|
|
||
| if (!argc) | ||
| return error(_("'%s' failed: no command provided."), "git bisect run"); | ||
| get_terms(&terms); | ||
| if (get_terms(&terms, 1)) | ||
| return error(_("no terms defined")); | ||
| res = bisect_run(&terms, argc, argv); | ||
| free_terms(&terms); | ||
| return res; | ||
|
|
@@ -1475,7 +1492,8 @@ int cmd_bisect(int argc, | |
| usage_with_options(git_bisect_usage, options); | ||
|
|
||
| set_terms(&terms, "bad", "good"); | ||
| get_terms(&terms); | ||
| if (get_terms(&terms, 1)) | ||
| return error(_("no terms defined")); | ||
| if (check_and_set_terms(&terms, argv[0]) || | ||
| !one_of(argv[0], terms.term_good, terms.term_bad, NULL)) | ||
| usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1313,7 +1313,10 @@ static int show_editor(struct config_location_options *opts) | |
| else if (errno != EEXIST) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Jul 14, 2026 at 10:48:35PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/config.c b/builtin/config.c
> index 8d8ec0beea..1307fdb0d6 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -1313,7 +1313,10 @@ static int show_editor(struct config_location_options *opts)
> else if (errno != EEXIST)
> die_errno(_("cannot create configuration file %s"), config_file);
> }
> - launch_editor(config_file, NULL, NULL);
> + if (launch_editor(config_file, NULL, NULL)) {
> + free(config_file);
> + return -1;
> + }
All error paths in `launch_editor()` already print an error message, so
we indeed don't have to do anything but bubble up the error here.
Patrick |
||
| die_errno(_("cannot create configuration file %s"), config_file); | ||
| } | ||
| launch_editor(config_file, NULL, NULL); | ||
| if (launch_editor(config_file, NULL, NULL)) { | ||
| free(config_file); | ||
| return -1; | ||
| } | ||
| free(config_file); | ||
|
|
||
| return 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -290,7 +290,8 @@ static void process_parent(struct last_modified *lm, | |
| { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Skip unparsable commits by checking the return value and
> continuing to the next iteration (or returning early in
> process_parent). This matches the defensive pattern used in other
> revision walkers such as limit_list() and get_revision_internal().
> ...
> @@ -414,12 +415,14 @@ static int last_modified_run(struct last_modified *lm)
> * Otherwise, make sure that 'c' isn't reachable from anything
> * in the '--not' queue.
> */
> - repo_parse_commit(lm->rev.repo, c);
> + if (repo_parse_commit(lm->rev.repo, c))
> + continue;
Shouldn't this be
goto cleanup;
instead? 'n' pulled out of not_queue may be unparseable and when we
ignore it, don't we still want to clean up the active_paths slab for
commit 'c'?
> while ((n = prio_queue_get(¬_queue))) {
> struct commit_list *np;
>
> - repo_parse_commit(lm->rev.repo, n);
> + if (repo_parse_commit(lm->rev.repo, n))
> + continue;
>
> for (np = n->parents; np; np = np->next) {
> if (!(np->item->object.flags & PARENT2)) {There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Junio C Hamano <gitster@pobox.com> writes:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> ...
>> - repo_parse_commit(lm->rev.repo, c);
>> + if (repo_parse_commit(lm->rev.repo, c))
>> + continue;
>
> Shouldn't this be
>
> goto cleanup;
>
> instead? 'n' pulled out of not_queue may be unparseable and when we
> ignore it, don't we still want to clean up the active_paths slab for
> commit 'c'?
--- >8 ---
Subject: [PATCH] fixup! last-modified: handle repo_parse_commit() failures
https://lore.kernel.org/git/xmqqldbdqciy.fsf@gitster.g/
'n' pulled out of not_queue may be unparseable and when we ignore
it, we still want to clean up the active_paths slab for commit 'c'.
diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index fe012b0c2e..3846244dfc 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -416,7 +416,7 @@ static int last_modified_run(struct last_modified *lm)
* in the '--not' queue.
*/
if (repo_parse_commit(lm->rev.repo, c))
- continue;
+ goto cleanup;
while ((n = prio_queue_get(¬_queue))) {
struct commit_list *np;There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Junio,
On Sun, 19 Jul 2026, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> > ...
> >> - repo_parse_commit(lm->rev.repo, c);
> >> + if (repo_parse_commit(lm->rev.repo, c))
> >> + continue;
> >
> > Shouldn't this be
> >
> > goto cleanup;
> >
> > instead? 'n' pulled out of not_queue may be unparseable and when we
> > ignore it, don't we still want to clean up the active_paths slab for
> > commit 'c'?
Correct.
Thanks,
Johannes
>
> --- >8 ---
> Subject: [PATCH] fixup! last-modified: handle repo_parse_commit() failures
>
> https://lore.kernel.org/git/xmqqldbdqciy.fsf@gitster.g/
>
> 'n' pulled out of not_queue may be unparseable and when we ignore
> it, we still want to clean up the active_paths slab for commit 'c'.
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index fe012b0c2e..3846244dfc 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -416,7 +416,7 @@ static int last_modified_run(struct last_modified *lm)
> * in the '--not' queue.
> */
> if (repo_parse_commit(lm->rev.repo, c))
> - continue;
> + goto cleanup;
>
> while ((n = prio_queue_get(¬_queue))) {
> struct commit_list *np;
> |
||
| struct bitmap *active_p; | ||
|
|
||
| repo_parse_commit(lm->rev.repo, parent); | ||
| if (repo_parse_commit(lm->rev.repo, parent)) | ||
| return; | ||
| active_p = active_paths_for(lm, parent); | ||
|
|
||
| /* | ||
|
|
@@ -414,12 +415,14 @@ static int last_modified_run(struct last_modified *lm) | |
| * Otherwise, make sure that 'c' isn't reachable from anything | ||
| * in the '--not' queue. | ||
| */ | ||
| repo_parse_commit(lm->rev.repo, c); | ||
| if (repo_parse_commit(lm->rev.repo, c)) | ||
| goto cleanup; | ||
|
|
||
| while ((n = prio_queue_get(¬_queue))) { | ||
| struct commit_list *np; | ||
|
|
||
| repo_parse_commit(lm->rev.repo, n); | ||
| if (repo_parse_commit(lm->rev.repo, n)) | ||
| continue; | ||
|
|
||
| for (np = n->parents; np; np = np->next) { | ||
| if (!(np->item->object.flags & PARENT2)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset) | |
| ssize_t rc; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Jul 14, 2026 at 10:48:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/pread.c b/compat/pread.c
> index 484e6d4c71..ac7d058cb8 100644
> --- a/compat/pread.c
> +++ b/compat/pread.c
> @@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
> ssize_t rc;
>
> current_offset = lseek(fd, 0, SEEK_CUR);
> + if (current_offset < 0)
> + return -1;
>
> if (lseek(fd, offset, SEEK_SET) < 0)
> return -1;
Heh, funny. I wanted to complain about misindentation here, but your new
code is actually indented correctly. It's everything else in this file
that is indented with spaces.
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Wed, 15 Jul 2026, Patrick Steinhardt wrote:
> On Tue, Jul 14, 2026 at 10:48:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/compat/pread.c b/compat/pread.c
> > index 484e6d4c71..ac7d058cb8 100644
> > --- a/compat/pread.c
> > +++ b/compat/pread.c
> > @@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
> > ssize_t rc;
> >
> > current_offset = lseek(fd, 0, SEEK_CUR);
> > + if (current_offset < 0)
> > + return -1;
> >
> > if (lseek(fd, offset, SEEK_SET) < 0)
> > return -1;
>
> Heh, funny. I wanted to complain about misindentation here, but your new
> code is actually indented correctly. It's everything else in this file
> that is indented with spaces.
Heh. I did notice something odd going on, thinking that Opus ignored my
clear instructions about tab-indentation once again when I replaced the
spaces by tabs...
Ciao,
Johannes |
||
|
|
||
| current_offset = lseek(fd, 0, SEEK_CUR); | ||
| if (current_offset < 0) | ||
| return -1; | ||
|
|
||
| if (lseek(fd, offset, SEEK_SET) < 0) | ||
| return -1; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,7 +87,8 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block, | |
| REFTABLE_CALLOC_ARRAY(bw->zstream, 1); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> The function already uses REFTABLE_ZLIB_ERROR for deflate()
> failures later in the code path (lines 171, 199), so returning
> the same error code for deflateInit() failure is consistent.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> reftable/block.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 920b3f4486..ec81fd0493 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -87,7 +87,8 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
> REFTABLE_CALLOC_ARRAY(bw->zstream, 1);
> if (!bw->zstream)
> return REFTABLE_OUT_OF_MEMORY_ERROR;
> - deflateInit(bw->zstream, 9);
> + if (deflateInit(bw->zstream, 9) != Z_OK)
> + return REFTABLE_ZLIB_ERROR;
> }
Presumably bw->zstream occupies some memory allocated on the heap.
Does a failing deflateInit() release it? If not, do we leak memory
here? Or do we need
if (deflateInit(bw->zstream, 9) !+ Z_OK) {
REFTABLE_FREE_AND_NULL(bw->zstream);
return REFTABLE_ZLIB_ERROR;
}
here?
Noticing and returning an error is a good first step. The only
caller of it is reftable/writer.c:writer_reinit_block_writer(), and
it checks and relays the error code from here to its callers, but
not all callers of it check the error condition. The most blatant
offender being reftable_writer_new() that happily keeps going. I do
not know if we end up calling zlib on bw->zstream for such a broken
block_writer(), as I didn't trace the call graph fully myself.
Stepping back a bit, if REFTABLE_CALLOC_ARRAY() fails, bw->zstream
would be NULL, and a caller that does not check the return value of
writer_reinit_block_writer() would be holding a block writer whose
zstream is NULL. If the block writer is eventually passed to the
block_writer_release() function, we would call deflateEnd() on it.
|
||
| if (!bw->zstream) | ||
| return REFTABLE_OUT_OF_MEMORY_ERROR; | ||
| deflateInit(bw->zstream, 9); | ||
| if (deflateInit(bw->zstream, 9) != Z_OK) | ||
| return REFTABLE_ZLIB_ERROR; | ||
| } | ||
|
|
||
| return 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,6 +487,8 @@ static int get_exporter(struct transport *transport, | |
| /* we need to duplicate helper->in because we want to use it after | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Jul 14, 2026 at 10:48:40PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/transport-helper.c b/transport-helper.c
> index 80f90eb7ba..31883b244e 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -487,6 +487,8 @@ static int get_exporter(struct transport *transport,
> /* we need to duplicate helper->in because we want to use it after
> * fastexport is done with it. */
> fastexport->out = dup(helper->in);
> + if (fastexport->out < 0)
> + return error_errno(_("could not dup helper output fd"));
> strvec_push(&fastexport->args, "fast-export");
> strvec_push(&fastexport->args, "--use-done-feature");
> strvec_push(&fastexport->args, data->signed_tags ?
Makes sense. The only caller already knows to die in case it sees a
non-zero return value.
Patrick |
||
| * fastexport is done with it. */ | ||
| fastexport->out = dup(helper->in); | ||
| if (fastexport->out < 0) | ||
| return error_errno(_("could not dup helper output fd")); | ||
| strvec_push(&fastexport->args, "fast-export"); | ||
| strvec_push(&fastexport->args, "--use-done-feature"); | ||
| strvec_push(&fastexport->args, data->signed_tags ? | ||
|
|
@@ -1182,7 +1184,9 @@ static int push_refs_with_export(struct transport *transport, | |
|
|
||
| if (data->export_marks) { | ||
| strbuf_addf(&buf, "%s.tmp", data->export_marks); | ||
| rename(buf.buf, data->export_marks); | ||
| if (rename(buf.buf, data->export_marks)) | ||
| warning_errno(_("could not rename '%s' to '%s'"), | ||
| buf.buf, data->export_marks); | ||
| strbuf_release(&buf); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):