[PATCH 7/9] http: discard hash in dumb-http http_object_request

[Jeff King <peff@peff.net>]

Usually an object request results in finish_http_object_request()
calling git_hash_final_oid(), after we've received all of the data. But
if we hit an error, we'll bail early and free the http_object_request,
dropping the git_hash_ctx entirely.  This can cause a leak for hash
implementations that allocate memory in their context, like OpenSSL >=
3.0.

The obvious fix is for abort_http_object_request() to call
git_hash_discard(), under the assumption that every request is either
finished or aborted. But that's not quite true:

  1. Not everybody calls the abort function. Sometimes they jump
     straight to release_http_object_request(). So we'd have to put it
     there.

  2. After the finish function finalizes the hash, we can still
     encounter errors! In that case we end up aborting or releasing,
     and they must not discard that hash (since that would be a
     double-free).

So we'll keep a flag marking the validity of the hash_ctx field of the
request. The lifetime is simple: it is valid immediately after creation,
up until we call finalize. And then our release function can just
conditionally discard the hash based on that flag.

This fixes test failures in t5550 and t5619 when run with:

  make SANITIZE=leak \
       OPENSSL_SHA256=1 \
       GIT_TEST_DEFAULT_HASH=sha256 \
       test

The flag handling could be removed if the hash-discard function were
idempotent. This could be done easily-ish by having the underlying
hash functions (like the ones in sha256/openssl.h) set the context
pointer to NULL after free-ing. But it's something that every platform
implementation would have to remember to do, and the benefit for the
callers is not that huge (it would let us shave a few lines here and
probably in a few other spots).

Signed-off-by: Jeff King <peff@peff.net>
---
I think the "set to NULL" thing gets weird with gcrypt, too, which does
not even use a pointer (we typedef their libgcrypt handle into our own
context struct).

 http.c | 4 ++++
 http.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/http.c b/http.c
index b4e7b8d00b..63abbaae8a 100644
--- a/http.c
+++ b/http.c
@@ -2880,6 +2880,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	git_inflate_init(&freq->stream);
 
 	the_hash_algo->init_fn(&freq->c);
+	freq->hash_ctx_valid = 1;
 
 	freq->url = get_remote_object_url(base_url, hex, 0);
 
@@ -2988,6 +2989,7 @@ int finish_http_object_request(struct http_object_request *freq)
 	}
 
 	git_hash_final_oid(&freq->real_oid, &freq->c);
+	freq->hash_ctx_valid = 0;
 	if (freq->zret != Z_STREAM_END) {
 		unlink_or_warn(freq->tmpfile.buf);
 		return -1;
@@ -3028,6 +3030,8 @@ void release_http_object_request(struct http_object_request **freq_p)
 	curl_slist_free_all(freq->headers);
 	strbuf_release(&freq->tmpfile);
 	git_inflate_end(&freq->stream);
+	if (freq->hash_ctx_valid)
+		git_hash_discard(&freq->c);
 
 	free(freq);
 	*freq_p = NULL;
diff --git a/http.h b/http.h
index 729c51904d..6b0639150f 100644
--- a/http.h
+++ b/http.h
@@ -255,6 +255,7 @@ struct http_object_request {
 	struct object_id oid;
 	struct object_id real_oid;
 	struct git_hash_ctx c;
+	int hash_ctx_valid;
 	git_zstream stream;
 	int zret;
 	int rename;
-- 
2.55.0.418.g37da59dd42