From: Justin Tobler <jltobler@gmail.com>
Date: Mon, 04 Nov 2024 22:43:38 GMT
Subject: Re: [PATCH v2 15/22] combine-diff: fix leaking lost lines
Message-ID: <rbjmoxqrfpcpz2b5oqxcchuwpbvw4uz4ps4j7ctkd4ux5h4xwj@n4mngoog4ffd>
In-Reply-To: <76bbcb3fe301fe273578a71849f99953ea94695c.1729502824.git.ps@pks.im>

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
> 
>