Re: [PATCH v2 15/22] combine-diff: fix leaking lost lines
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Nov 5, 2024, 05:55 UTC
- Message-ID
- <ZymzN5TQK5cpGw5Z@pks.im>
- In-Reply-To
- <rbjmoxqrfpcpz2b5oqxcchuwpbvw4uz4ps4j7ctkd4ux5h4xwj@n4mngoog4ffd>
On Mon, Nov 04, 2024 at 04:43:38PM -0600, Justin Tobler wrote:
Show 30 quoted lines
> On 24/10/21 11:28AM, Patrick Steinhardt wrote:
> > The `cnt` variable tracks the number of lines in a patch diff. It can
> > happen though that there are no newlines, in which case we'd still end
> > up allocating our array of `sline`s. In fact, we always allocate it with
> > `cnt + 2` entries. But when we loop through the array to clear it at the
> > end of this function we only loop until `lno < cnt`, and thus we may not
> > end up releasing whatever the two extra `sline`s contain.
> >
> > Plug this memory leak.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > combine-diff.c | 2 +-
> > t/t4038-diff-combined.sh | 1 +
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/combine-diff.c b/combine-diff.c
> > index f6b624dc288..3c6d9507fec 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -1220,7 +1220,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> > }
> > free(result);
> >
> > - for (lno = 0; lno < cnt; lno++) {
> > + for (lno = 0; lno < cnt + 2; lno++) {
>
> From looking only at the code, its not very obvious to me where the "+2"
> comes from. Not really worth a reroll, but it might be nice to leave a
> note with some context.The code is quite convoluted and hard to read, agreed. In any case, we call `CALLOC_ARRAY(sline, st_add(cnt, 2))`, and as a comment further up mentions:
/* Even p_lno[cnt+1] is valid -- that is for the end line number for
* deletion hunk at the end.
*/This explains the +1. The second +1 seems to never be populated and acts more like a sentinel value.
I don't really think it makes a ton of sense to explain why the sline array is overallocated when freeing it, and we already do explain it. But I'll touch up the commit message a bit and sneak in a fix of the above comment to be properly formatted, which also gives the necessary context when reading the diff, only.
Patrick