http: discard hash in dumb-http http_object_request

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/http.c b/http.c
index 67c9c6fc60673d8f536979560b78a0aeabe71a0d..8b0de51984066176e56c701b2c3554e489919f4e 100644 (file)
--- a/http.c
+++ b/http.c
@@ -2806,6 +2806,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);
 
@@ -2913,6 +2914,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;
@@ -2953,6 +2955,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 f9ee888c3ed67ef391c0f97e55135a1b10db2df0..dc9700c1131117fb545471f2803075d5ddb1af3a 100644 (file)
--- a/http.h
+++ b/http.h
@@ -249,6 +249,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;