[PATCH v3 00/12] coverity: fix unchecked returns

This is the next batch of fixes in response to issues reported by Coverity.

Changes since v2:

 * Added a new commit to handle block-writer initialization errors (instead
   of ignoring them).
 * The bw->zstream attribute is now also deinitialized in the error case, as
   suggested by Junio.
 * The commit message of "reftable/block: check deflateInit() return value"
   was rephrased to stop suggesting that silent corruption by zlib would be
   possible before that patch: This turned out to be provably incorrect.
 * When aborting the bisect because dup2() failed, a left-over saved_stdout
   is now also cleaned up.

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 (12):
  http: die on curl_easy_duphandle failure in get_active_slot
  config: propagate launch_editor() failure in show_editor()
  reftable: handle block-writer initialization errors
  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                | 44 ++++++++++++++++++++++++---------
 builtin/config.c                |  5 +++-
 builtin/last-modified.c         |  9 ++++---
 compat/pread.c                  |  2 ++
 http.c                          |  2 ++
 reftable/block.c                |  5 +++-
 reftable/writer.c               |  8 +++++-
 t/unit-tests/u-reftable-table.c |  6 +++--
 transport-helper.c              |  6 ++++-
 10 files changed, 70 insertions(+), 23 deletions(-)


base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2179%2Fdscho%2Fcoverity-fixes-unchecked-returns-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2179/dscho/coverity-fixes-unchecked-returns-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2179

Range-diff vs v2:

  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:  c689148aef reftable: handle block-writer initialization errors
  3:  9bf7e737c7 !  4:  66953a65d0 reftable/block: check deflateInit() return value
     @@ Commit message
          z_stream is left in an undefined state.
      
          Subsequent deflate() calls in block_writer_finish() then operate
     -    on this uninitialized stream. Depending on the zlib
     -    implementation, this can produce silently corrupted compressed
     -    data (which would be written to the reftable file and discovered
     -    only when a later reader fails to inflate) or crash outright.
     +    on this uninitialized stream. Current zlib/zlib-ng versions handle
     +    such a stream gracefully, by returning `Z_STREAM_ERROR`, so in
     +    practice it would likely not result in catastrophic error.
      
     -    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.
     +    The function already uses REFTABLE_ZLIB_ERROR for deflate() failures
     +    later in the code path, so returning the same error code for
     +    deflateInit() failure is consistent.
      
          Pointed out by Coverity.
      
          Assisted-by: Claude Opus 4.6
     +    Helped-by: Junio C Hamano <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## reftable/block.c ##
     @@ reftable/block.c: int block_writer_init(struct block_writer *bw, uint8_t typ, ui
       		if (!bw->zstream)
       			return REFTABLE_OUT_OF_MEMORY_ERROR;
      -		deflateInit(bw->zstream, 9);
     -+		if (deflateInit(bw->zstream, 9) != Z_OK)
     ++		if (deflateInit(bw->zstream, 9) != Z_OK) {
     ++			REFTABLE_FREE_AND_NULL(bw->zstream);
      +			return REFTABLE_ZLIB_ERROR;
     ++		}
       	}
       
       	return 0;
  4:  711671c3ab =  5:  a49af20d30 reftable tests: check reftable_table_init_ref_iterator() return
  5:  72a74c76be =  6:  bf06239732 last-modified: handle repo_parse_commit() failures
  6:  f0b1e13979 =  7:  6e2295b8f0 compat/pread: check initial lseek for errors
  7:  0facb9e8ca =  8:  689bb48fe5 transport-helper: check dup() return in get_exporter
  8:  2b0e4f32fd =  9:  ad6ea19737 transport-helper: warn when export-marks file cannot be finalized
  9:  7f2b963103 = 10:  7db6ac2ab0 bisect: check strbuf_getline_lf return when reading terms
 10:  9a9103096a = 11:  aefdbe2bdf bisect: check get_terms return at all call sites
 11:  829cd82177 ! 12:  258dbb0fbd bisect: handle dup() failure when redirecting stdout
     @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, co
      +		if (saved_stdout < 0 ||
      +		    dup2(temporary_stdout_fd, 1) < 0) {
      +			res = error_errno(_("could not duplicate stdout"));
     ++			if (saved_stdout >= 0)
     ++				close(saved_stdout);
      +			close(temporary_stdout_fd);
      +			break;
      +		}

-- 
gitgitgadget