Re: [PATCH] clone: plug a miniscule leak

From: Derrick Stolee <derrickstolee@github.com>
Date: May 2, 2022, 13:43 UTC
Message-ID: <5e8eff95-d768-439d-e2df-d575a0038ffd@github.com>
In-Reply-To: <xmqqlevl4ysk.fsf@gitster.g>

On 5/1/2022 1:17 AM, Junio C Hamano wrote:

Show 23 quoted lines

>     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