> On 27 Aug 2017, at 09:37, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
>
> 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.
Oh :-( Thanks for detecting and fixing the leak.
How did you actually find it? Reading the code or did you use some
tool?
> 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.
>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> 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.
>
> pkt-line.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 7db911957..f364944b9 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -136,9 +136,10 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
> static int packet_write_fmt_1(int fd, int gently,
> const char *fmt, va_list args)
> {
> - struct strbuf buf = STRBUF_INIT;
> + static struct strbuf buf = STRBUF_INIT;
> ssize_t count;
>
> + strbuf_reset(&buf);
> format_packet(&buf, fmt, args);
> count = write_in_full(fd, buf.buf, buf.len);
> if (count == buf.len)
> --
> 2.14.1.151.g45c1275a3.dirty
Looks good to me!
- Lars
