Re: [RFC PATCH 1/6] leak fix: cache_put_path

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2023, 19:23 UTC
Message-ID: <xmqqk00lbc8k.fsf@gitster.g>
In-Reply-To: <20230213182134.2173280-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 23 quoted lines

> hashmap_put returns a pointer if the key was found and subsequently
> replaced. Free this pointer so it isn't leaked.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  submodule-config.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 4dc61b3a78..90cab34568 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -128,9 +128,11 @@ static void cache_put_path(struct submodule_cache *cache,
>  	unsigned int hash = hash_oid_string(&submodule->gitmodules_oid,
>  					    submodule->path);
>  	struct submodule_entry *e = xmalloc(sizeof(*e));
> +	struct hashmap_entry *replaced;
>  	hashmap_entry_init(&e->ent, hash);
>  	e->config = submodule;
> -	hashmap_put(&cache->for_path, &e->ent);
> +	replaced = hashmap_put(&cache->for_path, &e->ent);
> +	free(replaced);
>  }

Out of curiosity, I've checked all the grep hits from hashmap_put() in the codebase and this seems to be the only one. Everybody else either calls hashmap_put() only after hashmap_get() sees that there is no existing one, or unconditionally calls hashmap_put() and dies if an earlier registration is found.

The callers of oidmap_put() in sequencer.c I didn't check. There might be similar leaks there, or they may be safe---I dunno. But all other callers of oidmap_put() also seem to be safe.

Back to the patch itself.  The only caller of this function does

	if (submodule->path) {
		cache_remove_path(me->cache, submodule);
		free(submodule->path);
	}
	submodule->path = xstrdup(value);
	cache_put_path(me->cache, submodule);

It is curious how the same submodule->path is occupied by more than one submodule? Isn't that a configuration error we want to report to the user somehow (not necessarily error/die), instead of silently replacing with the "last one wins" precedence?

Assuming that the "last one wins" is the sensible thing to do, the change proposed by this patch does seem reasonable way to plug the leak.

Thanks.