| Message ID | 20240924220550.GU1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 747a71019c49d41f46f70562102869e947d944ad |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 06:05:50PM -0400, Jeff King wrote: > diff --git a/http-push.c b/http-push.c > index 52c53928a9..451f7d14bb 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -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; > } I was quite confused by the layout of this function, where we had another `return 1` at the end. It took me a second to realize that this is the error case for the `if (start_active_slot(slot))` condition further up. But we do already free the headers in that case, so we're good. Patric
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); }
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 <peff@peff.net> --- http-push.c | 5 +++++ 1 file changed, 5 insertions(+)