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

On Sun, Aug 27, 2017 at 09:09:15PM +0200, Martin Ågren wrote:

On 27 August 2017 at 20:21, Lars Schneider [off-list ref] wrote:

quoted

quoted

On 27 Aug 2017, at 09:37, Martin Ågren [off-list ref] 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?

Valgrind found it for me. Most leaks it reports are "ok" since we're
about to exit, it just takes more or less effort to realize that...

This is one more thing it would be nice to have as part of our
perf-testing framework. It will run two versions of Git across a battery
of tests and report on the runtime differences for each. It would be
great if it did the same for peak heap. The tricky thing is that you
sometimes need repositories that are exaggerated in size in one
dimension to notice the differences as significant. So I don't know if
we would need new tests for this, or if existing "this repo has a lot of
refs" tests would have caught this.

Another approach, of course, is to get valgrind (or asan) to a
zero-leaks-reported steady state, and then even small leak reports would
be worth investigating. I'm not sure how easy it is to get there, and
whether it's less work to actually plug free-on-exit leaks or somehow
suppress the reports. I think most free-on-exit false positives would
show up as "leaked but still reachable", whereas leaks like this that
can grow without bound mean would not be reachable (we throw away the
pointer to the memory at the end of the function).

-Peff