[PATCH v2 00/11] coverity: fix unchecked returns

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

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(&not_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);
     + 

-- 
gitgitgadget