Re: [PATCH] clone: plug a miniscule leak

[Derrick Stolee <derrickstolee@github.com>]

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