Skip to content

Commit a2d8ea5

Browse files
peffgitster
authored andcommitted
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>
1 parent 77f78b8 commit a2d8ea5

2 files changed

Lines changed: 5 additions & 0 deletions

File tree

http.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2806,6 +2806,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
28062806
git_inflate_init(&freq->stream);
28072807

28082808
the_hash_algo->init_fn(&freq->c);
2809+
freq->hash_ctx_valid = 1;
28092810

28102811
freq->url = get_remote_object_url(base_url, hex, 0);
28112812

@@ -2913,6 +2914,7 @@ int finish_http_object_request(struct http_object_request *freq)
29132914
}
29142915

29152916
git_hash_final_oid(&freq->real_oid, &freq->c);
2917+
freq->hash_ctx_valid = 0;
29162918
if (freq->zret != Z_STREAM_END) {
29172919
unlink_or_warn(freq->tmpfile.buf);
29182920
return -1;
@@ -2953,6 +2955,8 @@ void release_http_object_request(struct http_object_request **freq_p)
29532955
curl_slist_free_all(freq->headers);
29542956
strbuf_release(&freq->tmpfile);
29552957
git_inflate_end(&freq->stream);
2958+
if (freq->hash_ctx_valid)
2959+
git_hash_discard(&freq->c);
29562960

29572961
free(freq);
29582962
*freq_p = NULL;

http.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ struct http_object_request {
249249
struct object_id oid;
250250
struct object_id real_oid;
251251
struct git_hash_ctx c;
252+
int hash_ctx_valid;
252253
git_zstream stream;
253254
int zret;
254255
int rename;

0 commit comments

Comments
 (0)