| Message ID | 20240924220213.GO1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8bdb84ebbbfadf71ae1760e68be5422cbe4872c6 |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 06:02:13PM -0400, Jeff King wrote: > 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(). I had to read this paragraph multiple times to understand it, as I wondered why you did end up adding it to `release_http_object_request()` even though the paragraph claims that you cannot. But what you say is that you must _move_ the call, not add it, and that's what the patch does. So yeah, that does make sense. Patrick
On Thu, Sep 26, 2024 at 03:50:20PM +0200, Patrick Steinhardt wrote: > > 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(). > > I had to read this paragraph multiple times to understand it, as I > wondered why you did end up adding it to `release_http_object_request()` > even though the paragraph claims that you cannot. But what you say is > that you must _move_ the call, not add it, and that's what the patch > does. Yeah, I could see the confusion. It is really "We can't just add a git_inflate_end() call and be done. We must also...". I don't think it's worth a re-roll on its own, but if I do re-roll I'll try to clarify. -Peff
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;
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 <peff@peff.net> --- http.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)