[PATCH] clone: plug a miniscule leak

[PATCH] clone: plug a miniscule leak

[Junio C Hamano]

The remote_name variable is first assigned a copy of the value of
the "clone.defaultremotename" configuration variable and then by the
value of the "--origin" command line option.  The former is prepared
to see multiple instances of the variable by freeing the current
value of the variable before a copy of the newly discovered value
gets assigned to it.  The latter blindly assigned a copy of the new
value to it, thereby leaking the value read from the configuration.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    This was discovered by a recently removed bogus coccinelle
    rewrite rule---if you apply an incorrect change suggested by the
    bogus rewrite rule to unconditionally assign to remote_name a
    copy of option_origin, or NULL, the value read from the
    configuration would be lost and never be used, thereby breaking
    a test to ensure the configuration is used, instead of the
    default remote nickname "origin".

    Perhaps a Coccinelle rule like this might have caught similar
    leaks:

	@@
	expression E;
	expression V;
	@@
	- if (E)
	-   V = xstrdup(E);
	+ if (E) {
	+   free(V);
	+   V = xstrdup(E);
	+ }

    The fact that the result of xstrdup() is assigned to V is that V
    is meant to hold a pointer to an allocated piece of memory.

    With the preimage of the above semantic patch, it is reasonable
    to expect that V may be initialized to NULL or may be holding a
    pointer to a piece of allocated memory when the control reaches
    here, because otherwise, V will be either need to be freed (when
    E was not NULL, in which case we assigned the result of
    xstrdup() to it) or V has garbage that cannot be freed later.
   
 builtin/clone.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git c/builtin/clone.c w/builtin/clone.c
index 5231656379..194d50f75f 100644
--- c/builtin/clone.c
+++ w/builtin/clone.c
@@ -1106,8 +1106,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 * apply the remote name provided by --origin only after this second
 	 * call to git_config, to ensure it overrides all config-based values.
 	 */
-	if (option_origin != NULL)
+	if (option_origin != NULL) {
+		free(remote_name);
 		remote_name = xstrdup(option_origin);
+	}
 
 	if (remote_name == NULL)
 		remote_name = xstrdup("origin");

Re: [PATCH] clone: plug a miniscule leak

[Derrick Stolee]

On 5/1/2022 1:17 AM, Junio C Hamano wrote:
>     Perhaps a Coccinelle rule like this might have caught similar
>     leaks:
> 
> 	@@
> 	expression E;
> 	expression V;
> 	@@
> 	- if (E)
> 	-   V = xstrdup(E);
> 	+ if (E) {
> 	+   free(V);
> 	+   V = xstrdup(E);
> 	+ }
> 
>     The fact that the result of xstrdup() is assigned to V is that V
>     is meant to hold a pointer to an allocated piece of memory.
> 
>     With the preimage of the above semantic patch, it is reasonable
>     to expect that V may be initialized to NULL or may be holding a
>     pointer to a piece of allocated memory when the control reaches
>     here, because otherwise, V will be either need to be freed (when
>     E was not NULL, in which case we assigned the result of
>     xstrdup() to it) or V has garbage that cannot be freed later.

Initially, I did think "what if V is not initialized to NULL?" but
you are right that the code would already be broken in that case.

> -	if (option_origin != NULL)

This technically wouldn't hit your rule, since "E" isn't just the
variable name, as we typically do with our style. Is that something
that Coccinelle automatically simplifies?

> +	if (option_origin != NULL) {

Do you want to take this opportunity to drop the "!= NULL" here?

> +		free(remote_name);
>  		remote_name = xstrdup(option_origin);
> +	}
> >  	if (remote_name == NULL)

Or do you want to keep similar style from the surrounding code?

Either way, looks good to me.

Thanks,
-Stolee

Re: [PATCH] clone: plug a miniscule leak

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>> -	if (option_origin != NULL)
>
> This technically wouldn't hit your rule, since "E" isn't just the
> variable name, as we typically do with our style. Is that something
> that Coccinelle automatically simplifies?
>
>> +	if (option_origin != NULL) {
>
> Do you want to take this opportunity to drop the "!= NULL" here?
>
>> +		free(remote_name);
>>  		remote_name = xstrdup(option_origin);
>> +	}
>> >  	if (remote_name == NULL)
>
> Or do you want to keep similar style from the surrounding code?

I think that it is better to leave that particular clean-up to
the equals-null.cocci topic started by Elia; I know having them
separate would cause a merge conflict, but even if I change them
here, it will result in the same merge conflict anyway ;-)

Re: [PATCH] clone: plug a miniscule leak

[Philip Oakley]

CC'ing Elia as per the mention. Thread
https://lore.kernel.org/git/xmqqzgjzzwnd.fsf@gitster.g/t/#u.

On 02/05/2022 18:12, Junio C Hamano wrote:  <xmqqzgjzzwnd.fsf@gitster.g>
> Derrick Stolee <derrickstolee@github.com> writes:
>
>>> -	if (option_origin != NULL)
>> This technically wouldn't hit your rule, since "E" isn't just the
>> variable name, as we typically do with our style. Is that something
>> that Coccinelle automatically simplifies?
>>
>>> +	if (option_origin != NULL) {
>> Do you want to take this opportunity to drop the "!= NULL" here?
>>
>>> +		free(remote_name);
>>>  		remote_name = xstrdup(option_origin);
>>> +	}
>>>>  	if (remote_name == NULL)
>> Or do you want to keep similar style from the surrounding code?
> I think that it is better to leave that particular clean-up to
> the equals-null.cocci topic started by Elia; I know having them
> separate would cause a merge conflict, but even if I change them
> here, it will result in the same merge conflict anyway ;-)