This is the next batch of fixes in response to issues reported by Coverity.
Changes since v1:
* The last-modified patch is now more careful to clean up a commit slab
when parsing the commit failed.
* When the "good" bisect term was read successfully, but not the "bad" one,
the "good" one is now cleaned up.
* Instead of detecting failed get_terms() calls indirectly, the return
value is now checked.
* Failures when bisect_run() calls dup2() are now handled properly, too.
Johannes Schindelin (11):
http: die on curl_easy_duphandle failure in get_active_slot
config: propagate launch_editor() failure in show_editor()
reftable/block: check deflateInit() return value
reftable tests: check reftable_table_init_ref_iterator() return
last-modified: handle repo_parse_commit() failures
compat/pread: check initial lseek for errors
transport-helper: check dup() return in get_exporter
transport-helper: warn when export-marks file cannot be finalized
bisect: check strbuf_getline_lf return when reading terms
bisect: check get_terms return at all call sites
bisect: handle dup() failure when redirecting stdout
bisect.c | 6 +++--
builtin/bisect.c | 42 +++++++++++++++++++++++----------
builtin/config.c | 5 +++-
builtin/last-modified.c | 9 ++++---
compat/pread.c | 2 ++
http.c | 2 ++
reftable/block.c | 3 ++-
t/unit-tests/u-reftable-table.c | 6 +++--
transport-helper.c | 6 ++++-
9 files changed, 59 insertions(+), 22 deletions(-)
base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2179%2Fdscho%2Fcoverity-fixes-unchecked-returns-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2179/dscho/coverity-fixes-unchecked-returns-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2179
Range-diff vs v1:
1: e653255de1 = 1: e653255de1 http: die on curl_easy_duphandle failure in get_active_slot
2: 0692704d45 = 2: 0692704d45 config: propagate launch_editor() failure in show_editor()
3: 9bf7e737c7 = 3: 9bf7e737c7 reftable/block: check deflateInit() return value
4: 711671c3ab = 4: 711671c3ab reftable tests: check reftable_table_init_ref_iterator() return
5: f728be4dac ! 5: 72a74c76be last-modified: handle repo_parse_commit() failures
@@ Commit message
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## builtin/last-modified.c ##
@@ builtin/last-modified.c: static int last_modified_run(struct last_modified *lm)
*/
- repo_parse_commit(lm->rev.repo, c);
+ if (repo_parse_commit(lm->rev.repo, c))
-+ continue;
++ goto cleanup;
while ((n = prio_queue_get(¬_queue))) {
struct commit_list *np;
6: b31e0326e7 = 6: f0b1e13979 compat/pread: check initial lseek for errors
7: 1792042098 = 7: 0facb9e8ca transport-helper: check dup() return in get_exporter
8: 13ddcce053 = 8: 2b0e4f32fd transport-helper: warn when export-marks file cannot be finalized
9: 17c382fdf4 ! 9: 7f2b963103 bisect: check strbuf_getline_lf return when reading terms
@@ Commit message
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## bisect.c ##
@@ builtin/bisect.c: static int get_terms(struct bisect_terms *terms)
- 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);
10: c0827a7947 ! 10: 9a9103096a bisect: check get_terms return at all call sites
@@ Commit message
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.
+ Let's not discard the return value, but handle an error with the same
+ message `bisect_terms()` already uses when reading the terms failed.
Pointed out by Coverity.
+ 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.
+
Assisted-by: Claude Opus 4.6
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## builtin/bisect.c ##
+@@ builtin/bisect.c: static int bisect_next_check(const struct bisect_terms *terms,
+ return decide_next(terms, current_term, !state.nr_good, !state.nr_bad);
+ }
+
+-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;
+@@ builtin/bisect.c: 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;
+ }
+
+@@ builtin/bisect.c: finish:
+
+ 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) {
@@ builtin/bisect.c: 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 */
- get_terms(terms);
-+ if (!terms->term_bad || !terms->term_good)
+- get_terms(terms);
++ if (get_terms(terms, 1))
+ return error(_("no terms defined"));
if (check_and_set_terms(terms, p))
return -1;
@@ builtin/bisect.c: 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 (!terms.term_bad || !terms.term_good)
+- get_terms(&terms);
++ if (get_terms(&terms, 1))
+ return error(_("no terms defined"));
res = bisect_next(&terms, prefix);
free_terms(&terms);
return res;
@@ builtin/bisect.c: 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 (!terms.term_bad || !terms.term_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;
@@ builtin/bisect.c: static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
+ int res;
struct bisect_terms terms = { 0 };
- get_terms(&terms);
-+ if (!terms.term_bad || !terms.term_good)
+- get_terms(&terms);
++ if (get_terms(&terms, 1))
+ return error(_("no terms defined"));
res = bisect_visualize(&terms, argc, argv);
free_terms(&terms);
return res;
@@ builtin/bisect.c: 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 (!terms.term_bad || !terms.term_good)
+- get_terms(&terms);
++ if (get_terms(&terms, 1))
+ return error(_("no terms defined"));
res = bisect_run(&terms, argc, argv);
free_terms(&terms);
return res;
@@ builtin/bisect.c: int cmd_bisect(int argc,
+ usage_with_options(git_bisect_usage, options);
set_terms(&terms, "bad", "good");
- get_terms(&terms);
-+ if (!terms.term_bad || !terms.term_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))
11: 2da452e39c ! 11: 829cd82177 bisect: handle dup() failure when redirecting stdout
@@ Commit message
leaving the process with stdout still pointing at the temporary file
for the remainder of the run.
- Treat a failed dup(1) as a fatal error for this bisect step: close
- the temporary file descriptor, report the error via error_errno(),
- and break out of the loop so the existing cleanup path handles the
- rest, just as on other failure paths in this function.
+ Treat a failed dup(1) or dup2(..., 1) as a fatal error for this bisect
+ step: close the temporary file descriptor, report the error via
+ error_errno(), and break out of the loop so the existing cleanup path
+ handles the rest, just as on other failure paths in this function.
Reported by Coverity as CID 1508242 ("Improper use of negative
value").
Assisted-by: Opus 4.7
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## builtin/bisect.c ##
@@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, co
fflush(stdout);
saved_stdout = dup(1);
-+ if (saved_stdout < 0) {
+- 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;
+ }
- dup2(temporary_stdout_fd, 1);
res = bisect_state(terms, 1, &new_state);
+