[PATCH] send-pack: clean up extra_have oid array

[PATCH] send-pack: clean up extra_have oid array

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Commit c8009635785e ("fetch-pack, send-pack: clean up shallow oid
array", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,
but didn't clean up extra_have, which is still leaked at program exit.
I suspect the particular tests in t5539 don't trigger any additions to
the extra_have array, which explains why the tests can pass leak free
despite this gap.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
I didn't check to see why the t5539 tests don't leak. This leak occured for
me in a day-to-day run with my local git build that happened to still have
sanitizers enabled:

=================================================================
==2930359==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2160 byte(s) in 1 object(s) allocated from:
    #0 0x7f51af6e5e2b in realloc.part.0 (/lib64/libasan.so.8+0xe5e2b) (BuildId: 7f1aa7e2e600e8c9d54ce6e3d36f3d31bfe7949a)
    #1 0x0000010dfc26 in xrealloc ../wrapper.c:140
    #2 0x000000c5d231 in oid_array_append ../oid-array.c:9
    #3 0x00000096036a in process_ref ../connect.c:296
    #4 0x00000096036a in get_remote_heads ../connect.c:374
    #5 0x00000072f8fc in cmd_send_pack ../builtin/send-pack.c:290
    #6 0x0000007d74d4 in run_builtin ../git.c:480
    #7 0x0000007d74d4 in handle_builtin ../git.c:746
    #8 0x0000007dbeb5 in run_argv ../git.c:813
    #9 0x0000007dbeb5 in cmd_main ../git.c:953
    #10 0x000000441dbf in main ../common-main.c:9
    #11 0x7f51aec115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4) (BuildId: 2b3c02fe7e4d3811767175b6f323692a10a4e116)
    #12 0x7f51aec116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7) (BuildId: 2b3c02fe7e4d3811767175b6f323692a10a4e116)
    #13 0x0000004440b4 in _start (/home/jekeller/libexec/git-core/git+0x4440b4) (BuildId: 6cd37a01505f2d67a4e7d39fd9f813b683be0300)

SUMMARY: AddressSanitizer: 2160 byte(s) leaked in 1 allocation(s)
---
 builtin/send-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c6e0e9d05186..61486e378cab 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
 	free_refs(remote_refs);
 	free_refs(local_refs);
 	refspec_clear(&rs);
+	oid_array_clear(&extra_have);
 	oid_array_clear(&shallow);
 	clear_cas_option(&cas);
 	return ret;

---
base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77
change-id: 20250627-jk-fix-leak-send-pack-e4787600cf60

Best regards,
--  
Jacob Keller <jacob.keller@gmail.com>

Re: [PATCH] send-pack: clean up extra_have oid array

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Commit c8009635785e ("fetch-pack, send-pack: clean up shallow oid
> array", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,
> but didn't clean up extra_have, which is still leaked at program exit.
> I suspect the particular tests in t5539 don't trigger any additions to
> the extra_have array, which explains why the tests can pass leak free
> despite this gap.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> I didn't check to see why the t5539 tests don't leak. This leak occured for
> me in a day-to-day run with my local git build that happened to still have
> sanitizers enabled:

The other side may tell you about objects you _cannot_ fetch from
them, but if you have them, these objects can participate in the
common ancestor discovery and reduce the size of the transfer.

If the repository A you are pushing into use an alternate object
store B (i.e., created by "git clone --reference B $URL A" to make A
borrow from another local repository B) for example, the refs in
that alternate B that point at objects not in the repository A are
shown as "extra" objects.

Perhaps we can have these tests push into such a repository?

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index c6e0e9d05186..61486e378cab 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
>  	free_refs(remote_refs);
>  	free_refs(local_refs);
>  	refspec_clear(&rs);
> +	oid_array_clear(&extra_have);
>  	oid_array_clear(&shallow);
>  	clear_cas_option(&cas);
>  	return ret;

The change looks obviously correct.

Thanks.

Re: [PATCH] send-pack: clean up extra_have oid array

[Jacob Keller]

On Mon, Jun 30, 2025 at 7:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > From: Jacob Keller <jacob.keller@gmail.com>
> >
> > Commit c8009635785e ("fetch-pack, send-pack: clean up shallow oid
> > array", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,
> > but didn't clean up extra_have, which is still leaked at program exit.
> > I suspect the particular tests in t5539 don't trigger any additions to
> > the extra_have array, which explains why the tests can pass leak free
> > despite this gap.
> >
> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> > ---
> > I didn't check to see why the t5539 tests don't leak. This leak occured for
> > me in a day-to-day run with my local git build that happened to still have
> > sanitizers enabled:
>
> The other side may tell you about objects you _cannot_ fetch from
> them, but if you have them, these objects can participate in the
> common ancestor discovery and reduce the size of the transfer.
>
> If the repository A you are pushing into use an alternate object
> store B (i.e., created by "git clone --reference B $URL A" to make A
> borrow from another local repository B) for example, the refs in
> that alternate B that point at objects not in the repository A are
> shown as "extra" objects.
>
> Perhaps we can have these tests push into such a repository?
>

I probably won't personally have time to work on extending these tests.

Thanks,
Jake

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index c6e0e9d05186..61486e378cab 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
> >       free_refs(remote_refs);
> >       free_refs(local_refs);
> >       refspec_clear(&rs);
> > +     oid_array_clear(&extra_have);
> >       oid_array_clear(&shallow);
> >       clear_cas_option(&cas);
> >       return ret;
>
> The change looks obviously correct.
>
> Thanks.

Re: [PATCH] send-pack: clean up extra_have oid array

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index c6e0e9d05186..61486e378cab 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
>  	free_refs(remote_refs);
>  	free_refs(local_refs);
>  	refspec_clear(&rs);
> +	oid_array_clear(&extra_have);
>  	oid_array_clear(&shallow);
>  	clear_cas_option(&cas);
>  	return ret;

There is an early exit from the function that would bypass these
clean-up.  Perhaps something like this on top?

 builtin/send-pack.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git c/builtin/send-pack.c w/builtin/send-pack.c
index b28da7ddd7..6ce9f6665a 100644
--- c/builtin/send-pack.c
+++ w/builtin/send-pack.c
@@ -305,9 +305,10 @@ int cmd_send_pack(int argc,
 		flags |= MATCH_REFS_MIRROR;
 
 	/* match them up */
-	if (match_push_refs(local_refs, &remote_refs, &rs, flags))
-		return -1;
-
+	if (match_push_refs(local_refs, &remote_refs, &rs, flags)) {
+		ret = -1;
+		goto cleanup;
+	}
 	if (!is_empty_cas(&cas))
 		apply_push_cas(&cas, remote, remote_refs);
 
@@ -340,6 +341,7 @@ int cmd_send_pack(int argc,
 		/* stable plumbing output; do not modify or localize */
 		fprintf(stderr, "Everything up-to-date\n");
 
+cleanup:
 	string_list_clear(&push_options, 0);
 	free_refs(remote_refs);
 	free_refs(local_refs);

Re: [PATCH] send-pack: clean up extra_have oid array

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1622 bytes --]



On 7/1/2025 10:40 AM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
>> index c6e0e9d05186..61486e378cab 100644
>> --- a/builtin/send-pack.c
>> +++ b/builtin/send-pack.c
>> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
>>  	free_refs(remote_refs);
>>  	free_refs(local_refs);
>>  	refspec_clear(&rs);
>> +	oid_array_clear(&extra_have);
>>  	oid_array_clear(&shallow);
>>  	clear_cas_option(&cas);
>>  	return ret;
> 
> There is an early exit from the function that would bypass these
> clean-up.  Perhaps something like this on top?
> 
>  builtin/send-pack.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git c/builtin/send-pack.c w/builtin/send-pack.c
> index b28da7ddd7..6ce9f6665a 100644
> --- c/builtin/send-pack.c
> +++ w/builtin/send-pack.c
> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,
>  		flags |= MATCH_REFS_MIRROR;
>  
>  	/* match them up */
> -	if (match_push_refs(local_refs, &remote_refs, &rs, flags))
> -		return -1;
> -
> +	if (match_push_refs(local_refs, &remote_refs, &rs, flags)) {
> +		ret = -1;
> +		goto cleanup;
> +	}
>  	if (!is_empty_cas(&cas))
>  		apply_push_cas(&cas, remote, remote_refs);
>  
> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,
>  		/* stable plumbing output; do not modify or localize */
>  		fprintf(stderr, "Everything up-to-date\n");
>  
> +cleanup:
>  	string_list_clear(&push_options, 0);
>  	free_refs(remote_refs);
>  	free_refs(local_refs);

This addition looks good to me.

Thanks,
Jake

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH] send-pack: clean up extra_have oid array

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> On 7/1/2025 10:40 AM, Junio C Hamano wrote:
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>> 
>>> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
>>> index c6e0e9d05186..61486e378cab 100644
>>> --- a/builtin/send-pack.c
>>> +++ b/builtin/send-pack.c
>>> @@ -343,6 +343,7 @@ int cmd_send_pack(int argc,
>>>  	free_refs(remote_refs);
>>>  	free_refs(local_refs);
>>>  	refspec_clear(&rs);
>>> +	oid_array_clear(&extra_have);
>>>  	oid_array_clear(&shallow);
>>>  	clear_cas_option(&cas);
>>>  	return ret;
>> 
>> There is an early exit from the function that would bypass these
>> clean-up.  Perhaps something like this on top?
>> 
>>  builtin/send-pack.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git c/builtin/send-pack.c w/builtin/send-pack.c
>> index b28da7ddd7..6ce9f6665a 100644
>> --- c/builtin/send-pack.c
>> +++ w/builtin/send-pack.c
>> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,
>>  		flags |= MATCH_REFS_MIRROR;
>>  
>>  	/* match them up */
>> -	if (match_push_refs(local_refs, &remote_refs, &rs, flags))
>> -		return -1;
>> -
>> +	if (match_push_refs(local_refs, &remote_refs, &rs, flags)) {
>> +		ret = -1;
>> +		goto cleanup;
>> +	}
>>  	if (!is_empty_cas(&cas))
>>  		apply_push_cas(&cas, remote, remote_refs);
>>  
>> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,
>>  		/* stable plumbing output; do not modify or localize */
>>  		fprintf(stderr, "Everything up-to-date\n");
>>  
>> +cleanup:
>>  	string_list_clear(&push_options, 0);
>>  	free_refs(remote_refs);
>>  	free_refs(local_refs);
>
> This addition looks good to me.

Thanks for a quick sanity check.  I'll queue it on top of yours,
then.

Re: [PATCH] send-pack: clean up extra_have oid array

[Jeff King]

On Fri, Jun 27, 2025 at 03:09:04PM -0700, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Commit c8009635785e ("fetch-pack, send-pack: clean up shallow oid
> array", 2024-09-25) cleaned up the shallow oid array in cmd_send_pack,
> but didn't clean up extra_have, which is still leaked at program exit.
> I suspect the particular tests in t5539 don't trigger any additions to
> the extra_have array, which explains why the tests can pass leak free
> despite this gap.

Thanks, this looks good. At the time I did that other commit, I was
focused on just bug-hunting the leaks reported by the tests. So I missed
this one.

> I didn't check to see why the t5539 tests don't leak. This leak occured for
> me in a day-to-day run with my local git build that happened to still have
> sanitizers enabled:

The tests are leak-free now, but I suspect we have a lot of
slightly-exotic command invocations like this that still leak. It might
be nice to beef up the test coverage, but I'm OK with just fixing them,
too.

-Peff

Re: [PATCH] send-pack: clean up extra_have oid array

[Jeff King]

On Tue, Jul 01, 2025 at 10:40:21AM -0700, Junio C Hamano wrote:

> There is an early exit from the function that would bypass these
> clean-up.  Perhaps something like this on top?
> 
>  builtin/send-pack.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git c/builtin/send-pack.c w/builtin/send-pack.c
> index b28da7ddd7..6ce9f6665a 100644
> --- c/builtin/send-pack.c
> +++ w/builtin/send-pack.c
> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,
>  		flags |= MATCH_REFS_MIRROR;
>  
>  	/* match them up */
> -	if (match_push_refs(local_refs, &remote_refs, &rs, flags))
> -		return -1;
> -
> +	if (match_push_refs(local_refs, &remote_refs, &rs, flags)) {
> +		ret = -1;
> +		goto cleanup;
> +	}
>  	if (!is_empty_cas(&cas))
>  		apply_push_cas(&cas, remote, remote_refs);
>  
> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,
>  		/* stable plumbing output; do not modify or localize */
>  		fprintf(stderr, "Everything up-to-date\n");
>  
> +cleanup:
>  	string_list_clear(&push_options, 0);
>  	free_refs(remote_refs);
>  	free_refs(local_refs);

This made me wonder if the remote_refs out-parameter is valid after
match_push_refs() returns failure (especially since we do not initialize
it at the top of the function).

I think the answer is "yes"; it is both an in-parameter and an
out-parameter, and will have been earlier set up via get_remote_heads().
So even on the failure case, match_push_refs() will leave it untouched
and it is still valid (and needs to be cleaned up).

-Peff

Re: [PATCH] send-pack: clean up extra_have oid array

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]



On 7/3/2025 8:40 AM, Jeff King wrote:
> On Tue, Jul 01, 2025 at 10:40:21AM -0700, Junio C Hamano wrote:
> 
>> There is an early exit from the function that would bypass these
>> clean-up.  Perhaps something like this on top?
>>
>>  builtin/send-pack.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git c/builtin/send-pack.c w/builtin/send-pack.c
>> index b28da7ddd7..6ce9f6665a 100644
>> --- c/builtin/send-pack.c
>> +++ w/builtin/send-pack.c
>> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc,
>>  		flags |= MATCH_REFS_MIRROR;
>>  
>>  	/* match them up */
>> -	if (match_push_refs(local_refs, &remote_refs, &rs, flags))
>> -		return -1;
>> -
>> +	if (match_push_refs(local_refs, &remote_refs, &rs, flags)) {
>> +		ret = -1;
>> +		goto cleanup;
>> +	}
>>  	if (!is_empty_cas(&cas))
>>  		apply_push_cas(&cas, remote, remote_refs);
>>  
>> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc,
>>  		/* stable plumbing output; do not modify or localize */
>>  		fprintf(stderr, "Everything up-to-date\n");
>>  
>> +cleanup:
>>  	string_list_clear(&push_options, 0);
>>  	free_refs(remote_refs);
>>  	free_refs(local_refs);
> 
> This made me wonder if the remote_refs out-parameter is valid after
> match_push_refs() returns failure (especially since we do not initialize
> it at the top of the function).
> 
> I think the answer is "yes"; it is both an in-parameter and an
> out-parameter, and will have been earlier set up via get_remote_heads().
> So even on the failure case, match_push_refs() will leave it untouched
> and it is still valid (and needs to be cleaned up).
> 

This was my assumption too.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]