Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()
- From
Jeff King <peff@peff.net>
- Date
- Aug 27, 2017, 19:15 UTC
- Message-ID
- <20170827191508.bkydifqihq3ke3rw@sigill.intra.peff.net>
- In-Reply-To
- <CAN0heSraJFbbog7FKpAtmob9W6_5-AS1StZFVW6xUwMDWfMYgg@mail.gmail.com>
On Sun, Aug 27, 2017 at 09:09:15PM +0200, Martin Ågren wrote:
Show 15 quoted lines
> On 27 August 2017 at 20:21, Lars Schneider <larsxschneider@gmail.com> wrote:
> >
> >> On 27 Aug 2017, at 09:37, Martin Ågren <martin.agren@gmail.com> 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