From 3d33e96653fecbc692fa5674b127fd598ad2dbeb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 17:59:14 -0400 Subject: http: fix leak when redacting cookies from curl trace When redacting headers for GIT_TRACE_CURL, we build up a redacted cookie header in a local strbuf, and then copy it into the output. But we forget to release the temporary strbuf, leaking it for every cookie header we show. The other redacted headers don't run into this problem, since they're able to work in-place in the output buffer. But the cookie parsing is too complicated for that, since we redact the cookies individually. This leak is triggered by the cookie tests in t5551. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 1 + 1 file changed, 1 insertion(+) (limited to 'http.c') diff --git a/http.c b/http.c index 6c6cc5c822..cc136408c0 100644 --- a/http.c +++ b/http.c @@ -800,6 +800,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset) strbuf_setlen(header, sensitive_header - header->buf); strbuf_addbuf(header, &redacted_header); + strbuf_release(&redacted_header); ret = 1; } return ret; -- cgit v1.2.3 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-push.c | 4 ++-- http-walker.c | 8 ++++---- http.c | 11 ++++++++--- http.h | 4 ++-- t/t5551-http-fetch-smart.sh | 1 + 5 files changed, 17 insertions(+), 11 deletions(-) (limited to 'http.c') diff --git a/http-push.c b/http-push.c index 7315a694aa..7196ffa525 100644 --- a/http-push.c +++ b/http-push.c @@ -275,7 +275,7 @@ static void start_fetch_loose(struct transfer_request *request) if (!start_active_slot(slot)) { fprintf(stderr, "Unable to start GET request\n"); repo->can_update_info_refs = 0; - release_http_object_request(obj_req); + release_http_object_request(&obj_req); release_request(request); } } @@ -580,7 +580,7 @@ static void finish_request(struct transfer_request *request) /* Try fetching packed if necessary */ if (request->obj->flags & LOCAL) { - release_http_object_request(obj_req); + release_http_object_request(&obj_req); release_request(request); } else start_fetch_packed(request); 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; } diff --git a/http.c b/http.c index cc136408c0..d0242ffb50 100644 --- a/http.c +++ b/http.c @@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq) return freq->rename; } -void abort_http_object_request(struct http_object_request *freq) +void abort_http_object_request(struct http_object_request **freq_p) { + struct http_object_request *freq = *freq_p; unlink_or_warn(freq->tmpfile.buf); - release_http_object_request(freq); + release_http_object_request(freq_p); } -void release_http_object_request(struct http_object_request *freq) +void release_http_object_request(struct http_object_request **freq_p) { + struct http_object_request *freq = *freq_p; if (freq->localfile != -1) { close(freq->localfile); freq->localfile = -1; @@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq) } curl_slist_free_all(freq->headers); strbuf_release(&freq->tmpfile); + + free(freq); + *freq_p = NULL; } diff --git a/http.h b/http.h index a516ca4a9a..46e334c2c2 100644 --- a/http.h +++ b/http.h @@ -240,8 +240,8 @@ struct http_object_request *new_http_object_request( const char *base_url, const struct object_id *oid); void process_http_object_request(struct http_object_request *freq); int finish_http_object_request(struct http_object_request *freq); -void abort_http_object_request(struct http_object_request *freq); -void release_http_object_request(struct http_object_request *freq); +void abort_http_object_request(struct http_object_request **freq); +void release_http_object_request(struct http_object_request **freq); /* * Instead of using environment variables to determine if curl tracing happens, diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 7b5ab0eae1..e36dfde17e 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -5,6 +5,7 @@ test_description="test smart fetching over http via http-backend ($HTTP_PROTO)" GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh test "$HTTP_PROTO" = "HTTP/2" && enable_http2 -- cgit v1.2.3 From 8bdb84ebbbfadf71ae1760e68be5422cbe4872c6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:02:13 -0400 Subject: http: call git_inflate_end() when releasing http_object_request In new_http_object_request(), we initialize the zlib stream with git_inflate_init(). We must have a matching git_inflate_end() to avoid leaking any memory allocated by zlib. In most cases this happens in finish_http_object_request(), but we don't always get there. If we abort a request mid-stream, then we may clean it up without hitting that function. We can't just add a git_inflate_end() call to the release function, though. That would double-free the cases that did actually finish. Instead, we'll move the call from the finish function to the release function. This does delay it for the cases that do finish, but I don't think it matters. We should have already reached Z_STREAM_END (and complain if we didn't), and we do not record any status code from git_inflate_end(). This leak is triggered by t5550 at least (and probably other dumb-http tests). I did find one other related spot of interest. If we try to read a previously downloaded file and fail, we reset the stream by calling memset() followed by a fresh git_inflate_init(). I don't think this case is triggered in the test suite, but it seemed like an obvious leak, so I added the appropriate git_inflate_end() before the memset() there. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'http.c') diff --git a/http.c b/http.c index d0242ffb50..4d841becca 100644 --- a/http.c +++ b/http.c @@ -2726,6 +2726,7 @@ struct http_object_request *new_http_object_request(const char *base_url, * file; also rewind to the beginning of the local file. */ if (prev_read == -1) { + git_inflate_end(&freq->stream); memset(&freq->stream, 0, sizeof(freq->stream)); git_inflate_init(&freq->stream); the_hash_algo->init_fn(&freq->c); @@ -2799,7 +2800,6 @@ int finish_http_object_request(struct http_object_request *freq) return -1; } - git_inflate_end(&freq->stream); the_hash_algo->final_oid_fn(&freq->real_oid, &freq->c); if (freq->zret != Z_STREAM_END) { unlink_or_warn(freq->tmpfile.buf); @@ -2840,6 +2840,7 @@ 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); free(freq); *freq_p = NULL; -- cgit v1.2.3 From 75f4acc98125a9e9f5b3051c0dc6690839830e25 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:02:27 -0400 Subject: http: stop leaking buffer in http_get_info_packs() We use http_get_strbuf() to fetch the remote info/packs content into a strbuf, but never free it, causing a leak. There's no need to hold onto it, as we've already parsed it completely. This lets us mark t5619 as leak-free. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 1 + t/t5619-clone-local-ambiguous-transport.sh | 1 + 2 files changed, 2 insertions(+) (limited to 'http.c') diff --git a/http.c b/http.c index 4d841becca..54463770b4 100644 --- a/http.c +++ b/http.c @@ -2475,6 +2475,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head) cleanup: free(url); + strbuf_release(&buf); return ret; } diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh index cce62bf78d..1d4efe414d 100755 --- a/t/t5619-clone-local-ambiguous-transport.sh +++ b/t/t5619-clone-local-ambiguous-transport.sh @@ -2,6 +2,7 @@ test_description='test local clone with ambiguous transport' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-httpd.sh" -- cgit v1.2.3