patch, 28 partsleak fixes for http fetch/push

Patrick asked me to take a look at the remaining leaks in the http push/fetch code, especially the dumb variants. So here are enough patches to all of these scripts running leak-free:

  t5500-fetch-pack.sh
  t5539-fetch-http-shallow.sh
  t5540-http-push-webdav.sh
  t5541-http-push-smart.sh
  t5542-push-http-shallow.sh
  t5550-http-fetch-dumb.sh
  t5551-http-fetch-smart.sh
  t5582-fetch-negative-refspec.sh
  t5619-clone-local-ambiguous-transport.sh

I did have to cheat a little and steal some of the patches from his "leak fixes part 8" work-in-progress (which are at the front of this series). But otherwise this should be independent of the other leak-fixing work and can be applied directly on master.

I tried to put patches touching similar areas together, but there's probably still a bit of a jumble, just because I got there by picking at the pile of leak logs for each script. :)

Most of them are pretty obvious "add a free call" one-liners. If you really want to put on your thinking cap, try patches 7, 14, and 21 (I'm going to pretend I spaced them out evenly for your reading pleasure).

  [01/28]: http-fetch: clear leaking git-index-pack(1) arguments
  [02/28]: shallow: fix leak when unregistering last shallow root
  [03/28]: fetch-pack: fix leaking sought refs
  [04/28]: connect: clear child process before freeing in diagnostic mode
  [05/28]: fetch-pack: free object filter before exiting
  [06/28]: fetch-pack, send-pack: clean up shallow oid array
  [07/28]: commit: avoid leaking already-saved buffer
  [08/28]: send-pack: free cas options before exit
  [09/28]: transport-helper: fix strbuf leak in push_refs_with_push()
  [10/28]: fetch: free "raw" string when shrinking refspec
  [11/28]: fetch-pack: clear pack lockfiles list
  [12/28]: transport-helper: fix leak of dummy refs_list
  [13/28]: http: fix leak when redacting cookies from curl trace
  [14/28]: http: fix leak of http_object_request struct
  [15/28]: http: call git_inflate_end() when releasing http_object_request
  [16/28]: http: stop leaking buffer in http_get_info_packs()
  [17/28]: remote-curl: free HEAD ref with free_one_ref()
  [18/28]: http-walker: free fake packed_git list
  [19/28]: http-push: clear refspecs before exiting
  [20/28]: http-push: free repo->url string
  [21/28]: http-push: free curl header lists
  [22/28]: http-push: free transfer_request dest field
  [23/28]: http-push: free transfer_request strbuf
  [24/28]: http-push: free remote_ls_ctx.dentry_name
  [25/28]: http-push: free xml_ctx.cdata after use
  [26/28]: http-push: clean up objects list
  [27/28]: http-push: clean up loose request when falling back to packed
  [28/28]: http-push: clean up local_refs at exit

 builtin/fetch-pack.c                       | 14 +++++++-
 builtin/fetch.c                            |  1 +
 builtin/push.c                             |  1 +
 builtin/send-pack.c                        |  2 ++
 commit.c                                   |  3 +-
 connect.c                                  |  1 +
 http-fetch.c                               | 16 ++++++---
 http-push.c                                | 40 +++++++++++++++-------
 http-walker.c                              | 18 +++++++---
 http.c                                     | 16 ++++++---
 http.h                                     |  4 +--
 refspec.c                                  |  2 +-
 refspec.h                                  |  2 +-
 remote-curl.c                              |  2 +-
 remote.c                                   |  2 +-
 remote.h                                   |  1 +
 shallow.c                                  |  5 ++-
 t/t5500-fetch-pack.sh                      |  1 +
 t/t5539-fetch-http-shallow.sh              |  1 +
 t/t5540-http-push-webdav.sh                |  1 +
 t/t5541-http-push-smart.sh                 |  1 +
 t/t5542-push-http-shallow.sh               |  1 +
 t/t5550-http-fetch-dumb.sh                 |  1 +
 t/t5551-http-fetch-smart.sh                |  1 +
 t/t5582-fetch-negative-refspec.sh          |  1 +
 t/t5619-clone-local-ambiguous-transport.sh |  1 +
 t/t5700-protocol-v1.sh                     |  1 +
 transport-helper.c                         | 11 ++++--
 transport.c                                | 11 +++++-
 29 files changed, 123 insertions(+), 39 deletions(-)

-Peff

From: Patrick Steinhardt <ps@pks.im>

We never clear the arguments that we pass to git-index-pack(1). Create a common exit path and release them there to plug this leak.

This is leak is exposed by t5702, but plugging the leak does not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously I didn't write this or the next two patches, but FWIW, I
reviewed them and they all look good to me.

 http-fetch.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

From: Patrick Steinhardt <ps@pks.im>

When unregistering a shallow root we shrink the array of grafts by one and move remaining grafts one to the left. This can of course only happen when there are any grafts left, because otherwise there is nothing to move. As such, this code is guarded by a condition that only performs the move in case there are grafts after the position of the graft to be unregistered.

By mistake we also put the call to free the unregistered graft into that condition. But that doesn't make any sense, as we want to always free the graft when it exists. Fix the resulting memory leak by doing so.

This leak is exposed by t5500, but plugging it does not make the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
 shallow.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

From: Patrick Steinhardt <ps@pks.im>

When calling `fetch_pack()` the caller is expected to pass in a set of sought-after refs that they want to fetch. This array gets massaged to not contain duplicate entries, which is done by replacing duplicate refs with `NULL` pointers. This modifies the caller-provided array, and in case we do unset any pointers the caller now loses track of that ref and cannot free it anymore.

Now the obvious fix would be to not only unset these pointers, but to also free their contents. But this doesn't work because callers continue to use those refs. Another potential solution would be to copy the array in `fetch_pack()` so that we dont modify the caller-provided one. But that doesn't work either because the NULL-ness of those entries is used by callers to skip over ref entries that we didn't even try to fetch in `report_unmatched_refs()`.

Instead, we make it the responsibility of our callers to duplicate these arrays as needed. It ain't pretty, but it works to plug the memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c   | 11 ++++++++++-
 t/t5700-protocol-v1.sh |  1 +
 transport.c            | 11 ++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

Am 24.09.24 um 23:51 schrieb Jeff King:

OK.

This makes a shallow copy and passes it to fetch_pack() and later to report_unmatched_refs(). It shields callers of fetch_refs_via_pack() from the effect of fetch_pack()'s NULLification. This is what the commit message says would not work. What am I missing?

On Wed, Sep 25, 2024 at 07:17:24PM +0200, René Scharfe wrote:

`fetch_refs_via_pack()` is called via `transport_fetch_refs()`, which constructs the array of refs to fetch ad-hoc and then discards it immediately. So it doesn't ever inspect the modified array in the first place, and consequently we can guard against the NULLification here.

This code is quite intricate :/

Patrick

The git_connect() function has a special CONNECT_DIAG_URL mode, where we stop short of actually connecting to the other side and just print some parsing details. For URLs that require a child process (like ssh), we free() the child_process struct but forget to clear it, leaking the strings we stuffed into its "env" list.

This leak is triggered many times in t5500, which uses "fetch-pack --diag-url", but we're not yet ready to mark it as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 connect.c | 1 +
 1 file changed, 1 insertion(+)

On Tue, Sep 24, 2024 at 05:51:24PM -0400, Jeff King wrote:

There's only a single exit path in this function that ends up discarding the `struct child_process`, so this looks good to me.

Patrick

Our fetch_pack_args holds a filter_options struct that may be populated with allocated strings by the by the "--filter" command-line option. We must free it before exiting to avoid a leak when the program exits.

The usual fetch code paths that use transport.c don't have the same leak, because we do the cleanup in disconnect_git().

Fixing this leak lets us mark t5500 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c  | 1 +
 t/t5500-fetch-pack.sh | 1 +
 2 files changed, 2 insertions(+)

On Tue, Sep 24, 2024 at 05:52:07PM -0400, Jeff King wrote:

s/by the by the/by the/

This fix seems familiar to me, I think I also had it at one point in time. Cannot find it in any of my series though, so... *shrug* Anyway, this looks good to me.

Patrick

When we call get_remote_heads() for protocol v0, that may populate the "shallow" oid_array, which must be cleaned up to avoid a leak at the program exit. The same problem exists for both fetch-pack and send-pack, but not for the usual transport.c code paths, since we already do this cleanup in disconnect_git().

Fixing this lets us mark t5542 as leak-free for the send-pack side, but fetch-pack will need some more fixes before we can do the same for t5539.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c         | 1 +
 builtin/send-pack.c          | 1 +
 t/t5542-push-http-shallow.sh | 1 +
 3 files changed, 3 insertions(+)

On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote:

I wonder about the early exit path we have when `finish_connect()` returns non-zero. Should we make it go via the common cleanup path as well, or do we not care in the error case?

We also have an early exit in this function when `match_push_refs()` returns non-zero.

Patrick

On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote:

Yeah, I think it would technically be a leak if we hit that error case. But it's pretty unlikely in practice (you'd have to finish the fetch and then the "ssh" or "upload-pack" sub-process returns a non-zero exit code anyway).

I'd prefer not to deal with it here for two reasons:

  1. The same leak is true of all the existing cleanup. So it really is
     orthogonal to what this patch is doing, and should be a separate
     patch.

  2. I think cleaning up in top-level builtins like this is mostly
     cosmetic to appease the leak checker. In the real world the memory
     is going back to the OS when we return anyway. Even in a libified
     world, I don't think cmd_fetch_pack() is a reasonable entry point
     (we already have a more lib-ish fetch_pack() that is used by the
     transport code).

So I don't think it would be wrong to refactor the cleanup to trigger on that other early return. But I also suspect there are many such paths lurking (and will continue to lurk, even after we run clean with SANITIZE=leak) and I'm not sure how productive it is to hunt them down.

And I think this is in the same boat.

-Peff

When we parse a commit via repo_parse_commit_internal(), if save_commit_buffer is set we'll stuff the buffer of the object contents into a cache, overwriting any previous value.

This can result in a leak of that previously cached value, though it's rare in practice. If we have a value in the cache it would have come from a previous parse, and during that parse we'd set the object.parsed flag, causing any subsequent parse attempts to exit without doing any work.

But it's possible to "unparse" a commit, which we do when registering a commit graft. And since shallow fetches are implemented using grafts, the leak is triggered in practice by t5539.

There are a number of possible ways to address this:

  1. the unparsing function could clear the cached commit buffer, too. I
     think this would work for the case I found, but I'm not sure if
     there are other ways to end up in the same state (an unparsed
     commit with an entry in the commit buffer cache).

  2. when we parse, we could check the buffer cache and prefer it to
     reading the contents from the object database. In theory the
     contents of a particular sha1 are immutable, but the code in
     question is violating the immutability with grafts. So this
     approach makes me a bit nervous, although I think it would work in
     practice (the grafts are applied to what we parse, but we still
     retain the original contents).

  3. We could realize the cache is already populated and discard its
     contents before overwriting. It's possible some other code could be
     holding on to a pointer to the old cache entry (and we'd introduce
     a use-after-free), but I think the risk of that is relatively low.

  4. The reverse of (3): when the cache is populated, don't bother
     saving our new copy. This is perhaps a little weird, since we'll
     have just populated the commit struct based on a different buffer.
     But the two buffers should be the same, even in the presence of
     grafts (as in (2) above).

I went with option 4. It addresses the leak directly and doesn't carry any risk of breaking other assumptions. And it's the same technique used by parse_object_buffer() for this situation, though I'm not sure when it would even come up there. The extra safety has been there since bd1e17e245 (Make "parse_object()" also fill in commit message buffer data., 2005-05-25).

This lets us mark t5539 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.c                      | 3 ++-
 t/t5539-fetch-http-shallow.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

On Tue, Sep 24, 2024 at 05:54:34PM -0400, Jeff King wrote:

Interesting. I saw some cases that I think could be caused by this, but couldn't make much sense of them.

s/the/The/

s/when/When/

Okay. This feels a bit weird indeed, but the fact that we already use the same strategy in other places makes me feel way safer.

And the fix is trivial.

Patrick

The send-pack --force-with-lease option populates a push_cas_option struct with allocated strings. Exiting without cleaning this up will cause leak-checkers to complain.

We can fix this by calling clear_cas_option(), after making it publicly available. Previously it was used only for resetting the list when we saw --no-force-with-lease.

The git-push command has the same "leak", though in this case it won't trigger a leak-checker since it stores the push_cas_option struct as a global rather than on the stack (and is thus reachable even after main() exits). I've added cleanup for it here anyway, though, as future-proofing.

The leak is triggered by t5541 (it tests --force-with-lease over http, which requires a separate send-pack process under the hood), but we can't mark it as leak-free yet.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/push.c      | 1 +
 builtin/send-pack.c | 1 +
 remote.c            | 2 +-
 remote.h            | 1 +
 4 files changed, 4 insertions(+), 1 deletion(-)

On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote:

Nit: I was wondering whether we'd also want to fix up this functions
name to conform to our style guide, which says this should be called
`push_cas_option_clear()` instead. But I don't mind it much, so please
feel free to ignore this nit.

Patrick

On Thu, Sep 26, 2024 at 03:50:09PM +0200, Patrick Steinhardt wrote:

I'd prefer to punt on that for now, as the whole suite of "methods" for this struct would need to be renamed to match that style. If we were making a too-short name into a public symbol, I'd be worried about addressing that now, but I think this is purely about style and can wait.

-Peff

We loop over the refs to push, building up a strbuf with the set of "push" directives to send to the remote helper. But if the atomic-push flag is set and we hit a rejected ref, we'll bail from the function early. We clean up most things, but forgot to release the strbuf.

Fixing this lets us mark t5541 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
Arguably this whole function could benefit from a cleanup goto, but it's
a little non-trivial. So I stuck with simple-and-stupid.

 t/t5541-http-push-smart.sh | 1 +
 transport-helper.c         | 1 +
 2 files changed, 2 insertions(+)

On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:

What's not visible here is that a few lines further down we have another early return where we don't clear `buf`. But that exit is conditioned on `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we know that `buf.len == 0` only when it hasn't ever be allocated.

So this LGTM.

Patrick

On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote:

Yeah, I noticed that, too, and came to the same conclusion. As you note, it's _possible_ to have an allocated but zero-length strbuf, but I don't think it happens here. So it's OK in practice.

If we were going to refactor, I think it would make sense to do it with a cleanup label to cover both of these early returns, plus the cleanup at the end. I was mostly just going for minimal changes where possible.

-Peff

The "--prefetch" option to git-fetch modifies the default refspec, including eliminating some entries entirely. When we drop an entry we free the strings in the refspec_item, but we forgot to free the matching string in the "raw" array of the refspec struct. There's no behavioral bug here (since we correctly shrink the raw array, too), but we're leaking the allocated string.

Let's add in the leak-fix, and while we're at it drop "const" from the type of the raw string array. These strings are always allocated by refspec_append(), etc, and this makes the memory ownership more clear.

This is all a bit more intimate with the refspec code than I'd like, and I suspect it would be better if each refspec_item held on to its own raw string, we had a single array, and we could use refspec_item_clear() to clean up everything. But that's a non-trivial refactoring, since refspec_item structs can be held outside of a "struct refspec", without having a matching raw string at all. So let's leave that for now and just fix the leak in the most immediate way.

This lets us mark t5582 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c                   | 1 +
 refspec.c                         | 2 +-
 refspec.h                         | 2 +-
 t/t5582-fetch-negative-refspec.sh | 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

If the --lock-pack option is passed (which it typically is when fetch-pack is used under the hood by smart-http), then we may end up with entries in our pack_lockfiles string_list. We need to clear them before returning to avoid a leak.

In git-fetch this isn't a problem, since the same cleanup happens via transport_unlock_pack(). But the leak is detectable in t5551, which does http fetches.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c | 1 +
 1 file changed, 1 insertion(+)

When using a remote-helper, the fetch_refs() function will issue a "list" command if we haven't already done so. We don't care about the result, but this is just to maintain compatibility as explained in ac3fda82bf (transport-helper: skip ls-refs if unnecessary, 2019-08-21).

But get_refs_list_using_list(), the function we call to issue the command, does parse and return the resulting ref list, which we simply leak. We should record the return value and free it immediately (another approach would be to teach it to avoid allocating at all, but it does not seem worth the trouble to micro-optimize this mostly historical case).

Triggering this requires the v0 protocol (since in v2 we use stateless connect to take over the connection). You can see it in t5551.37, "fetch by SHA-1 without tag following", as it explicitly enables v0.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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 <peff@peff.net>
---
 http.c | 1 +
 1 file changed, 1 insertion(+)

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 <peff@peff.net>
---
 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(-)

On Tue, Sep 24, 2024 at 06:01:09PM -0400, Jeff King wrote:

Oh yeah, I remember staring at this code and being completely confused as to how this all works.

Okay, looks simple enough. But I found the whole code in "http.c" to be quite... elusive, so I had a hard time finding my way.

Patrick

On Thu, Sep 26, 2024 at 03:50:17PM +0200, Patrick Steinhardt wrote:

You are not the only one. ;)

-Peff

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(-)

On Tue, Sep 24, 2024 at 06:02:13PM -0400, Jeff King wrote:

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:

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

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 <peff@peff.net>
---
 http.c                                     | 1 +
 t/t5619-clone-local-ambiguous-transport.sh | 1 +
 2 files changed, 2 insertions(+)

After dumb-http downloads the remote info/refs file, it adds an extra HEAD ref struct to our list by downloading the remote symref and finding the matching ref within our list. If either of those fails, we throw away the ref struct. But we do so with free(), when we should use free_one_ref() to catch any embedded allocations (in particular, if fetching the remote HEAD succeeded but the branch is unborn, its ref->symref field will be populated but we'll still throw it all away).

This leak is triggered by t5550 (but we still have a little more work to mark it leak-free).

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The dumb-http walker code creates a "fake" packed_git list representing packs we've downloaded from the remote (I call it "fake" because generally that struct is only used and managed by the local repository struct). But during our cleanup phase we don't touch those at all, causing a leak.

There's no support here from the rest of the object-database API, as these structs are not meant to be freed, except when closing the object store completely. But we can see that raw_object_store_clear() just calls free() on them, and that's enough here to fix the leak.

I also added a call to close_pack() before each. In the regular code this happens via close_object_store(), which we do as part of raw_object_store_clear(). This is necessary to prevent leaking mmap'd data (like the pack idx) or descriptors. The leak-checker won't catch either of these itself, but I did confirm with some hacky warning() calls and running t5550 that it's easy to leak at least index data.

This is all much more intimate with the packed_git struct than I'd like, but I think fixing it would be a pretty big refactor. And it's just not worth it for dumb-http code which is rarely used these days. If we can silence the leak-checker without creating too much hassle, we should just do that.

This lets us mark t5550 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c              | 10 ++++++++++
 t/t5550-http-fetch-dumb.sh |  1 +
 2 files changed, 11 insertions(+)

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 <peff@peff.net>
---
 http-push.c | 2 ++
 1 file changed, 2 insertions(+)

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 <peff@peff.net>
---
 http-push.c | 1 +
 1 file changed, 1 insertion(+)

On Tue, Sep 24, 2024 at 06:04:46PM -0400, Jeff King wrote:

I was wondering whether we also need to free `repo->path`, which is a `char *` as well. But that is only being assigned pointers into command line argument strings, and thus we do not have to free it.

Patrick

On Thu, Sep 26, 2024 at 03:50:24PM +0200, Patrick Steinhardt wrote:

Yeah, I think that "path" has the wrong type, and should be "const char *". I was a little surprised the compiler does not complain, but it is the old C gotcha: we assign to it via strchr(), which launders away the const.

It would probably be OK to fix, though in general my dream is still that we'd delete all of this code in the not too distant future. Even if we keep dumb-http fetch for its resumability, dumb-http push just seems like a useless holdover from the early days.

-Peff

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(+)

On Tue, Sep 24, 2024 at 06:05:50PM -0400, Jeff King wrote:

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

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 <peff@peff.net>
---
 http-push.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

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 <peff@peff.net>
---
 http-push.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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 <peff@peff.net>
---
 http-push.c | 1 +
 1 file changed, 1 insertion(+)

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 <peff@peff.net>
---
 http-push.c | 2 ++
 1 file changed, 2 insertions(+)

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 <peff@peff.net>
---
 http-push.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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 <peff@peff.net>
---
 http-push.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 <peff@peff.net>
---
 http-push.c                 | 3 ++-
 t/t5540-http-push-webdav.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

On Tue, Sep 24, 2024 at 05:49:30PM -0400, Jeff King wrote:

Thank you for taking a look at this, highly appreciated! The series looks good to me, I've only got a couple of nits regarding typos that you may or may not want to address. Either way is fine with me.

Patrick

On Thu, Sep 26, 2024 at 03:50:34PM +0200, Patrick Steinhardt wrote:

I responded to a few, but I don't think any of them really merits a re-roll. Thanks for reviewing!

-Peff