git/http-push.c, branch v2.47.2

http-push: clean up local_refs at exit

2024-09-25T17:24:58Z

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: clean up loose request when falling back to packed

2024-09-25T17:24:58Z

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: clean up objects list

2024-09-25T17:24:57Z

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: free xml_ctx.cdata after use

2024-09-25T17:24:57Z

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: free remote_ls_ctx.dentry_name

2024-09-25T17:24:57Z

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: free transfer_request strbuf

2024-09-25T17:24:57Z

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: free transfer_request dest field

2024-09-25T17:24:57Z

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: free curl header lists

2024-09-25T17:24:56Z

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: free repo->url string

2024-09-25T17:24:56Z

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: clear refspecs before exiting

2024-09-25T17:24:56Z

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