From a1bc3c88de1526c83882143b2e47400f7e3ee4b2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:01:09 -0400 Subject: http: fix leak of http_object_request struct The new_http_object_request() function allocates a struct on the heap, along with some fields inside the struct. But the matching function to clean it up, release_http_object_request(), only frees the interior fields without freeing the struct itself, causing a leak. The related http_pack_request new/release pair gets this right, and at first glance we should be able to do the same thing and just add a single free() call. But there's a catch. These http_object_request structs are typically embedded in the object_request struct of http-walker.c. And when we clean up that parent struct, it sanity-checks the embedded struct to make sure we are not leaking descriptors. Which means a use-after-free if we simply free() the embedded struct. I have no idea how valuable that sanity-check is, or whether it can simply be deleted. This all goes back to 5424bc557f (http*: add helper methods for fetching objects (loose), 2009-06-06). But the obvious way to make it all work is to be sure we set the pointer to NULL after freeing it (and our freeing process closes the descriptor, so we know there is no leak). To make sure we do that consistently, we'll switch the pointer we take in release_http_object_request() to a pointer-to-pointer, and we'll set it to NULL ourselves. And then the compiler can help us find each caller which needs to be updated. Most cases will just pass "&obj_req->req", which will obviously do the right thing. In a few cases, like http-push's finish_request(), we are working with a copy of the pointer, so we don't NULL the original. But it's OK because the next step is to free the struct containing the original pointer anyway. This lets us mark t5551 as leak-free. Ironically this is the "smart" http test, and the leak here only affects dumb http. But there's a single dumb-http invocation in there. The full dumb tests are in t5550, which still has some more leaks. This also makes t5559 leak-free, as it's just an HTTP/2 variant of t5551. But we don't need to mark it as such, since it inherits the flag from t5551. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-walker.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'http-walker.c') diff --git a/http-walker.c b/http-walker.c index e417a7f51c..9c1e5c37e6 100644 --- a/http-walker.c +++ b/http-walker.c @@ -74,7 +74,7 @@ static void start_object_request(struct object_request *obj_req) obj_req->state = ACTIVE; if (!start_active_slot(slot)) { obj_req->state = ABORTED; - release_http_object_request(req); + release_http_object_request(&req); return; } } @@ -110,7 +110,7 @@ static void process_object_response(void *callback_data) if (obj_req->repo->next) { obj_req->repo = obj_req->repo->next; - release_http_object_request(obj_req->req); + release_http_object_request(&obj_req->req); start_object_request(obj_req); return; } @@ -495,7 +495,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash) if (repo_has_object_file(the_repository, &obj_req->oid)) { if (obj_req->req) - abort_http_object_request(obj_req->req); + abort_http_object_request(&obj_req->req); abort_object_request(obj_req); return 0; } @@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash) strbuf_release(&buf); } - release_http_object_request(req); + release_http_object_request(&obj_req->req); release_object_request(obj_req); return ret; } -- cgit v1.2.3 From 134bfedf6de62dd7a0723a7100d5f05da505fe65 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:04:12 -0400 Subject: http-walker: free fake packed_git list The dumb-http walker code creates a "fake" packed_git list representing packs we've downloaded from the remote (I call it "fake" because generally that struct is only used and managed by the local repository struct). But during our cleanup phase we don't touch those at all, causing a leak. There's no support here from the rest of the object-database API, as these structs are not meant to be freed, except when closing the object store completely. But we can see that raw_object_store_clear() just calls free() on them, and that's enough here to fix the leak. I also added a call to close_pack() before each. In the regular code this happens via close_object_store(), which we do as part of raw_object_store_clear(). This is necessary to prevent leaking mmap'd data (like the pack idx) or descriptors. The leak-checker won't catch either of these itself, but I did confirm with some hacky warning() calls and running t5550 that it's easy to leak at least index data. This is all much more intimate with the packed_git struct than I'd like, but I think fixing it would be a pretty big refactor. And it's just not worth it for dumb-http code which is rarely used these days. If we can silence the leak-checker without creating too much hassle, we should just do that. This lets us mark t5550 as leak-free. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-walker.c | 10 ++++++++++ t/t5550-http-fetch-dumb.sh | 1 + 2 files changed, 11 insertions(+) (limited to 'http-walker.c') diff --git a/http-walker.c b/http-walker.c index 9c1e5c37e6..fb2d86d5e7 100644 --- a/http-walker.c +++ b/http-walker.c @@ -579,8 +579,18 @@ static void cleanup(struct walker *walker) if (data) { alt = data->alt; while (alt) { + struct packed_git *pack; + alt_next = alt->next; + pack = alt->packs; + while (pack) { + struct packed_git *pack_next = pack->next; + close_pack(pack); + free(pack); + pack = pack_next; + } + free(alt->base); free(alt); diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index ea8e48f627..58189c9f7d 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -4,6 +4,7 @@ test_description='test dumb fetching over http via static file' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if test_have_prereq !REFFILES -- cgit v1.2.3