Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

From: Jeff King <peff@peff.net>
Date: Aug 27, 2017, 15:41 UTC
Message-ID: <20170827154152.4n2ykna7e4l27fyn@sigill.intra.peff.net>
In-Reply-To: <20170827073732.546-1-martin.agren@gmail.com>

On Sun, Aug 27, 2017 at 09:37:32AM +0200, Martin Ågren wrote:

Show 10 quoted lines

> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
> packet_write_fmt_1, we allocate and leak a buffer.
> 
> We could keep the strbuf non-static and instead make sure we always
> release it before returning (but not before we die, so that we don't
> touch errno). That would also prepare us for threaded use. But until
> that needs to happen, let's just restore the static-ness so that we get
> back to a situation where we (eventually) do not continuosly keep
> allocating memory.

Ouch. So this means that git since v2.11 is basically leaking every non-byte pack sent by upload-pack (so all of the ref advertisement and want/have negotiation).

I'm surprised nobody noticed the extra memory use, but I guess those things aren't usually _too_ big.

Show 5 quoted lines

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> I waffled between "fixing the memory leak" by releasing the buffer and
> "fixing the static-ness" as in this patch. I can see arguments both ways
> and don't have any strong opinion.

I think this is a good fix for now as it takes as back to the pre-bug state. The only downside with the static buffer is that it's not reentrant. Since the function is just inherently writing out the result and then forgetting it, in a single thread there's no opportunity for a sub-function to try writing another packet. And I don't think we have any code paths that write packets from multiple threads. That may be something we do eventually, but we can deal with it then (and until then, it's nice to avoid the malloc/free overhead).

-Peff