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
