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

[Junio C Hamano <gitster@pobox.com>]

Calvin Wan <calvinwan@google.com> writes:

> 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.