patchclone: plug a miniscule leak

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(-)

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

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.

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?

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

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

Either way, looks good to me.

Thanks, -Stolee

Derrick Stolee <derrickstolee@github.com> writes:

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 ;-)

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>