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

Jeff King committed Sep 24, 2024 at 18:01 UTC a1bc3c88de1526c83882143b2e47400f7e3ee4b2
5 files changed +17 -11
http-push.c
+2 -2
@@ -275,7 +275,7 @@ static void start_fetch_loose(struct transfer_request *request)
275 if (!start_active_slot(slot)) {
276 fprintf(stderr, "Unable to start GET request\n");
277 repo->can_update_info_refs = 0;
278 - release_http_object_request(obj_req);
278 + release_http_object_request(&obj_req);
279 release_request(request);
280 }
281 }
@@ -580,7 +580,7 @@ static void finish_request(struct transfer_request *request)
580
581 /* Try fetching packed if necessary */
582 if (request->obj->flags & LOCAL) {
583 - release_http_object_request(obj_req);
583 + release_http_object_request(&obj_req);
584 release_request(request);
585 } else
586 start_fetch_packed(request);
http-walker.c
+4 -4
@@ -74,7 +74,7 @@ static void start_object_request(struct object_request *obj_req)
74 obj_req->state = ACTIVE;
75 if (!start_active_slot(slot)) {
76 obj_req->state = ABORTED;
77 - release_http_object_request(req);
77 + release_http_object_request(&req);
78 return;
79 }
80 }
@@ -110,7 +110,7 @@ static void process_object_response(void *callback_data)
110 if (obj_req->repo->next) {
111 obj_req->repo =
112 obj_req->repo->next;
113 - release_http_object_request(obj_req->req);
113 + release_http_object_request(&obj_req->req);
114 start_object_request(obj_req);
115 return;
116 }
@@ -495,7 +495,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)
495
496 if (repo_has_object_file(the_repository, &obj_req->oid)) {
497 if (obj_req->req)
498 - abort_http_object_request(obj_req->req);
498 + abort_http_object_request(&obj_req->req);
499 abort_object_request(obj_req);
500 return 0;
501 }
@@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)
543 strbuf_release(&buf);
544 }
545
546 - release_http_object_request(req);
546 + release_http_object_request(&obj_req->req);
547 release_object_request(obj_req);
548 return ret;
549 }
http.c
+8 -3
@@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq)
2816 return freq->rename;
2817 }
2818
2819 -void abort_http_object_request(struct http_object_request *freq)
2819 +void abort_http_object_request(struct http_object_request **freq_p)
2820 {
2821 + struct http_object_request *freq = *freq_p;
2822 unlink_or_warn(freq->tmpfile.buf);
2823
2823 - release_http_object_request(freq);
2824 + release_http_object_request(freq_p);
2825 }
2826
2826 -void release_http_object_request(struct http_object_request *freq)
2827 +void release_http_object_request(struct http_object_request **freq_p)
2828 {
2829 + struct http_object_request *freq = *freq_p;
2830 if (freq->localfile != -1) {
2831 close(freq->localfile);
2832 freq->localfile = -1;
@@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq)
2840 }
2841 curl_slist_free_all(freq->headers);
2842 strbuf_release(&freq->tmpfile);
2843 +
2844 + free(freq);
2845 + *freq_p = NULL;
2846 }
http.h
+2 -2
@@ -240,8 +240,8 @@ struct http_object_request *new_http_object_request(
240 const char *base_url, const struct object_id *oid);
241 void process_http_object_request(struct http_object_request *freq);
242 int finish_http_object_request(struct http_object_request *freq);
243 -void abort_http_object_request(struct http_object_request *freq);
244 -void release_http_object_request(struct http_object_request *freq);
243 +void abort_http_object_request(struct http_object_request **freq);
244 +void release_http_object_request(struct http_object_request **freq);
245
246 /*
247 * Instead of using environment variables to determine if curl tracing happens,
t/t5551-http-fetch-smart.sh
+1
@@ -5,6 +5,7 @@ test_description="test smart fetching over http via http-backend ($HTTP_PROTO)"
5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7
8 +TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10 . "$TEST_DIRECTORY"/lib-httpd.sh
11 test "$HTTP_PROTO" = "HTTP/2" && enable_http2