Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Dec 18, 2025

Thankfully, it is set to NULL, so no security consequences.

However, this is still a mistake that must be rectified.

Signed-off-by: Greg Funni gfunni234@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com

@AZero13
Copy link
Contributor Author

AZero13 commented Dec 18, 2025

/submit

@gitgitgadget-git
Copy link

Submitted as pull.2132.git.git.1766071566022.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-2132/AZero13/twice-v1

To fetch this version to local tag pr-git-2132/AZero13/twice-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2132/AZero13/twice-v1

Thankfully, it is set to NULL, so no security consequences.
However, this is still a mistake that must be rectified.

Signed-off-by: Greg Funni <gfunni234@gmail.com>
@AZero13 AZero13 changed the title repository: cache->squash_msg is freed twice repository: remove duplicate free of cache->squash_msg Dec 18, 2025
@AZero13
Copy link
Contributor Author

AZero13 commented Dec 18, 2025

/submit

@gitgitgadget-git
Copy link

Submitted as pull.2132.v2.git.git.1766072952115.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-2132/AZero13/twice-v2

To fetch this version to local tag pr-git-2132/AZero13/twice-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2132/AZero13/twice-v2

@gitgitgadget-git
Copy link

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Thu, Dec 18, 2025 at 10:26 AM AZero13 via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Thankfully, it is set to NULL, so no security consequences.
> However, this is still a mistake that must be rectified.
>
> Signed-off-by: Greg Funni <gfunni234@gmail.com>
> ---
> diff --git a/repository.c b/repository.c
> @@ -349,7 +349,6 @@ out:
>  static void repo_clear_path_cache(struct repo_path_cache *cache)
>  {
> -       FREE_AND_NULL(cache->squash_msg);
>         FREE_AND_NULL(cache->squash_msg);
>         FREE_AND_NULL(cache->merge_msg);
>         FREE_AND_NULL(cache->merge_rr);

This mistake has been present since Ævar added this function in
759f340738 (repository.c: free the "path cache" in repo_clear(),
2022-03-04), so it isn't the result of someone else coming along and
adding a new field to the structure which needs freeing but then
botching the call to FREE_AND_NULL(). Moreover, this function does
free all the freeable members of repo_path_cache, hence, nothing is
being leaked, so it must have just been a silly copy/paste mistake in
the first place. Hence, this change makes sense.

@gitgitgadget-git
Copy link

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

@gitgitgadget-git
Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Dec 18, 2025 at 10:26 AM AZero13 via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> Thankfully, it is set to NULL, so no security consequences.
>> However, this is still a mistake that must be rectified.
>>
>> Signed-off-by: Greg Funni <gfunni234@gmail.com>
>> ---
>> diff --git a/repository.c b/repository.c
>> @@ -349,7 +349,6 @@ out:
>>  static void repo_clear_path_cache(struct repo_path_cache *cache)
>>  {
>> -       FREE_AND_NULL(cache->squash_msg);
>>         FREE_AND_NULL(cache->squash_msg);
>>         FREE_AND_NULL(cache->merge_msg);
>>         FREE_AND_NULL(cache->merge_rr);
>
> This mistake has been present since Ævar added this function in
> 759f340738 (repository.c: free the "path cache" in repo_clear(),
> 2022-03-04), so it isn't the result of someone else coming along and
> adding a new field to the structure which needs freeing but then
> botching the call to FREE_AND_NULL(). Moreover, this function does
> free all the freeable members of repo_path_cache, hence, nothing is
> being leaked, so it must have just been a silly copy/paste mistake in
> the first place. Hence, this change makes sense.

Thanks, both.  Will queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant