| Message ID | 76bbcb3fe301fe273578a71849f99953ea94695c.1729502824.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.9) | expand |
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. > if (sline[lno].lost) { > struct lline *ll = sline[lno].lost; > while (ll) { > diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh > index 2ce26e585c9..00190802d83 100755 > --- a/t/t4038-diff-combined.sh > +++ b/t/t4038-diff-combined.sh > @@ -5,6 +5,7 @@ test_description='combined diff' > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-diff.sh > > -- > 2.47.0.72.gef8ce8f3d4.dirty > >
On Mon, Nov 04, 2024 at 04:43:38PM -0600, Justin Tobler wrote: > 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
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++) { if (sline[lno].lost) { struct lline *ll = sline[lno].lost; while (ll) { diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh index 2ce26e585c9..00190802d83 100755 --- a/t/t4038-diff-combined.sh +++ b/t/t4038-diff-combined.sh @@ -5,6 +5,7 @@ test_description='combined diff' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh
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(-)