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 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'http-push.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); -- cgit 1.2.3-korg From 85430af347a06b66932eec7c935def4558e0610f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:04:30 -0400 Subject: http-push: clear refspecs before exiting We parse the command-line arguments into a refspec struct, but we never free them. We should do so before exiting to avoid triggering the leak-checker. This triggers in t5540 many times (basically every invocation of http-push). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 7196ffa525..f60b2ceba5 100644 --- a/http-push.c +++ b/http-push.c @@ -1983,5 +1983,7 @@ int cmd_main(int argc, const char **argv) request = next_request; } + refspec_clear(&rs); + return rc; } -- cgit 1.2.3-korg From 4324c6c0d97ec48be18dfae8f1c1c3fb77d43f45 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:04:46 -0400 Subject: http-push: free repo->url string Our repo->url string comes from str_end_url_with_slash(), which always allocates its output buffer. We should free it before exiting to avoid triggering the leak-checker. This can be seen by leak-checking t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 1 + 1 file changed, 1 insertion(+) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index f60b2ceba5..52c53928a9 100644 --- a/http-push.c +++ b/http-push.c @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv) cleanup: if (info_ref_lock) unlock_remote(info_ref_lock); + free(repo->url); free(repo); http_cleanup(); -- cgit 1.2.3-korg From 747a71019c49d41f46f70562102869e947d944ad Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:05:50 -0400 Subject: http-push: free curl header lists To pass headers to curl, we have to allocate a curl_slist linked list and then feed it to curl_easy_setopt(). But the header list is not copied by curl, and must remain valid until we are finished with the request. A few spots in http-push get this right, freeing the list after finishing the request, but many do not. In most cases the fix is simple: we set up the curl slot, start it, and then use run_active_slot() to take it to completion. After that, we don't need the headers anymore and can call curl_slist_free_all(). But one case is trickier: when we do a MOVE request, we start the request but don't immediately finish it. It's possible we could change this to be more like the other requests, but I didn't want to get into risky refactoring of this code. So we need to stick the header list into the request struct and remember to free it later. Curiously, the struct already has a headers field for this purpose! It goes all the way back to 58e60dd203 (Add support for pushing to a remote repository using HTTP/DAV, 2005-11-02), but it doesn't look like it was ever used. We can make use of it just by assigning our headers to it, and there is already code in finish_request() to clean it up. This fixes several leaks triggered by t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 52c53928a9..451f7d14bb 100644 --- a/http-push.c +++ b/http-push.c @@ -437,9 +437,11 @@ static void start_move(struct transfer_request *request) if (start_active_slot(slot)) { request->slot = slot; request->state = RUN_MOVE; + request->headers = dav_headers; } else { request->state = ABORTED; FREE_AND_NULL(request->url); + curl_slist_free_all(dav_headers); } } @@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock) if (start_active_slot(slot)) { run_active_slot(slot); strbuf_release(&out_buffer.buf); + curl_slist_free_all(dav_headers); if (results.curl_result != CURLE_OK) { fprintf(stderr, "PUT error: curl result=%d, HTTP code=%ld\n", @@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock) } } else { strbuf_release(&out_buffer.buf); + curl_slist_free_all(dav_headers); fprintf(stderr, "Unable to start PUT request\n"); return 0; } @@ -1516,6 +1520,7 @@ static void update_remote_info_refs(struct remote_lock *lock) results.curl_result, results.http_code); } } + curl_slist_free_all(dav_headers); } strbuf_release(&buffer.buf); } -- cgit 1.2.3-korg From 7d3c71ddbf346bb5182d1cbb4610b9b575b34491 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:06:38 -0400 Subject: http-push: free transfer_request dest field When we issue a PUT request, we store the destination in the "dest" field by detaching from a strbuf. But we never free the result, causing a leak. We can address this in the release_request() function. But note that we also need to initialize it to NULL, as most other request types do not set it at all. Curiously there are _two_ functions to initialize a transfer_request struct. Adding the initialization only to add_fetch_request() seems to be enough for t5540, but I won't pretend to understand why. Rather than just adding "request->dest = NULL" in both spots, let's zero the whole struct. That addresses this problem, as well as any future ones (and it can't possibly hurt, as by definition we'd be hitting uninitialized memory previously). This fixes several leaks noticed by t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 451f7d14bb..9aa4d11ccd 100644 --- a/http-push.c +++ b/http-push.c @@ -514,6 +514,7 @@ static void release_request(struct transfer_request *request) } free(request->url); + free(request->dest); free(request); } @@ -651,11 +652,8 @@ static void add_fetch_request(struct object *obj) return; obj->flags |= FETCHING; - request = xmalloc(sizeof(*request)); + CALLOC_ARRAY(request, 1); request->obj = obj; - request->url = NULL; - request->lock = NULL; - request->headers = NULL; request->state = NEED_FETCH; request->next = request_queue_head; request_queue_head = request; @@ -687,11 +685,9 @@ static int add_send_request(struct object *obj, struct remote_lock *lock) } obj->flags |= PUSHING; - request = xmalloc(sizeof(*request)); + CALLOC_ARRAY(request, 1); request->obj = obj; - request->url = NULL; request->lock = lock; - request->headers = NULL; request->state = NEED_PUSH; request->next = request_queue_head; request_queue_head = request; -- cgit 1.2.3-korg From 94c62857808bdd6b5b061284eb9dfd13204bd11a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:08:49 -0400 Subject: http-push: free transfer_request strbuf When we issue a PUT, we initialize and fill a strbuf embedded in the transfer_request struct. But we never release this buffer, causing a leak. We can fix this by adding a strbuf_release() call to release_request(). If we stopped there, then non-PUT requests would try to release a zero-initialized strbuf. This works OK in practice, but we should try to follow the strbuf API more closely. So instead, we'll always initialize the strbuf when we create the transfer_request struct. That in turn means switching the strbuf_init() call in start_put() to a simple strbuf_grow(). This leak is triggered in t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 9aa4d11ccd..8acdb3f265 100644 --- a/http-push.c +++ b/http-push.c @@ -375,7 +375,7 @@ static void start_put(struct transfer_request *request) /* Set it up */ git_deflate_init(&stream, zlib_compression_level); size = git_deflate_bound(&stream, len + hdrlen); - strbuf_init(&request->buffer.buf, size); + strbuf_grow(&request->buffer.buf, size); request->buffer.posn = 0; /* Compress it */ @@ -515,6 +515,7 @@ static void release_request(struct transfer_request *request) free(request->url); free(request->dest); + strbuf_release(&request->buffer.buf); free(request); } @@ -655,6 +656,7 @@ static void add_fetch_request(struct object *obj) CALLOC_ARRAY(request, 1); request->obj = obj; request->state = NEED_FETCH; + strbuf_init(&request->buffer.buf, 0); request->next = request_queue_head; request_queue_head = request; @@ -689,6 +691,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock) request->obj = obj; request->lock = lock; request->state = NEED_PUSH; + strbuf_init(&request->buffer.buf, 0); request->next = request_queue_head; request_queue_head = request; -- cgit 1.2.3-korg From a1528093babd4c10415ece172235deb287ca7139 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:09:37 -0400 Subject: http-push: free remote_ls_ctx.dentry_name The remote_ls_ctx struct has dentry_name string, which is filled in with a heap allocation in the handle_remote_ls_ctx() XML callback. After the XML parse is done in remote_ls(), we should free the string to avoid a leak. This fixes several leaks found by running t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 1 + 1 file changed, 1 insertion(+) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 8acdb3f265..2e1c6851bb 100644 --- a/http-push.c +++ b/http-push.c @@ -1183,6 +1183,7 @@ static void remote_ls(const char *path, int flags, } free(ls.path); + free(ls.dentry_name); free(url); strbuf_release(&out_buffer.buf); strbuf_release(&in_buffer); -- cgit 1.2.3-korg From 3245a2ade5ee0ff3e5fad7bd96ad0a630c590e82 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:09:54 -0400 Subject: http-push: free xml_ctx.cdata after use When we ask libexpat to parse XML data, we sometimes set xml_cdata as a CharacterDataHandler callback. This fills in an allocated string in the xml_ctx struct which we never free, causing a leak. I won't pretend to understand the purpose of the field, but it looks like it is used by other callbacks during the parse. At any rate, we never look at it again after XML_Parse() returns, so we should be OK to free() it then. This fixes several leaks triggered by t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 2e1c6851bb..1146d7c6fe 100644 --- a/http-push.c +++ b/http-push.c @@ -913,6 +913,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) result = XML_Parse(parser, in_buffer.buf, in_buffer.len, 1); free(ctx.name); + free(ctx.cdata); if (result != XML_STATUS_OK) { fprintf(stderr, "XML error: %s\n", XML_ErrorString( @@ -1170,6 +1171,7 @@ static void remote_ls(const char *path, int flags, result = XML_Parse(parser, in_buffer.buf, in_buffer.len, 1); free(ctx.name); + free(ctx.cdata); if (result != XML_STATUS_OK) { fprintf(stderr, "XML error: %s\n", -- cgit 1.2.3-korg From 92e1eb491a9b8d47d8aa9e903354f7749fc85c4e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:10:38 -0400 Subject: http-push: clean up objects list In http-push's get_delta(), we generate a list of pending objects by recursively processing trees and blobs, adding them to a linked list. And then we iterate over the list, adding a new request for each element. But since we iterate using the list head pointer, at the end it is NULL and all of the actual list structs have been leaked. We can fix this either by using a separate iterator and then calling object_list_free(), or by just freeing as we go. I picked the latter, just because it means we continue to shrink the list as we go, though I'm not sure it matters in practice (we call add_send_request() in the loop, but I don't think it ever looks at the global objects list itself). This fixes several leaks noticed in t5540. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 1146d7c6fe..1cddd2fb37 100644 --- a/http-push.c +++ b/http-push.c @@ -1374,9 +1374,13 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock) } while (objects) { + struct object_list *next = objects->next; + if (!(objects->item->flags & UNINTERESTING)) count += add_send_request(objects->item, lock); - objects = objects->next; + + free(objects); + objects = next; } return count; -- cgit 1.2.3-korg From 9699327945d16f67f8a20a299d63621a3b1d3cd2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:11:13 -0400 Subject: http-push: clean up loose request when falling back to packed In http-push's finish_request(), if we fail a loose object request we may fall back to trying a packed request. But if we do so, we leave the http_loose_object_request in place, leaking it. We can fix this by always cleaning it up. Note that the obj_req pointer here (which we'll set to NULL) is a copy of the request->userData pointer, which will now point to freed memory. But that's OK. We'll either release the parent request struct entirely, or we'll convert it into a packed request, which will overwrite userData itself. This leak is found by t5540, but it's not quite leak-free yet. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index 1cddd2fb37..b36b1f9e35 100644 --- a/http-push.c +++ b/http-push.c @@ -582,9 +582,10 @@ static void finish_request(struct transfer_request *request) if (obj_req->rename == 0) request->obj->flags |= (LOCAL | REMOTE); + release_http_object_request(&obj_req); + /* Try fetching packed if necessary */ if (request->obj->flags & LOCAL) { - release_http_object_request(&obj_req); release_request(request); } else start_fetch_packed(request); -- cgit 1.2.3-korg From f4c768c639566da07fc063fa9b52607f54ac94ee Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Sep 2024 18:12:39 -0400 Subject: http-push: clean up local_refs at exit We allocate a list of ref structs from get_local_heads() but never clean it up. We should do so before exiting to avoid complaints from the leak-checker. Note that we have to initialize it to NULL, because there's one code path that can jump to the cleanup label before we assign to it. Fixing this lets us mark t5540 as leak-free. Curiously building with SANITIZE=leak and gcc does not seem to find this problem, but switching to clang does. It seems like a fairly obvious leak, though. I was curious that the matching remote_refs did not have the same leak. But that is because we store the list in a global variable, so it's still reachable after we exit. Arguably we could treat it the same as future-proofing, but I didn't bother (now that the script is marked leak-free, anybody moving it to a stack variable will notice). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 3 ++- t/t5540-http-push-webdav.sh | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) (limited to 'http-push.c') diff --git a/http-push.c b/http-push.c index b36b1f9e35..aad89f2eab 100644 --- a/http-push.c +++ b/http-push.c @@ -1719,7 +1719,7 @@ int cmd_main(int argc, const char **argv) int rc = 0; int i; int new_refs; - struct ref *ref, *local_refs; + struct ref *ref, *local_refs = NULL; CALLOC_ARRAY(repo, 1); @@ -1997,6 +1997,7 @@ int cmd_main(int argc, const char **argv) } refspec_clear(&rs); + free_refs(local_refs); return rc; } diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh index 37db3dec0c..27389b0908 100755 --- a/t/t5540-http-push-webdav.sh +++ b/t/t5540-http-push-webdav.sh @@ -10,6 +10,7 @@ This test runs various sanity checks on http-push.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if git http-push > /dev/null 2>&1 || [ $? -eq 128 ] -- cgit 1.2.3-korg