Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2018, 04:26 UTC
Message-ID: <xmqqin6z5g8e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180604035637.GA15408@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> I've looked into it before, but that causes its own wave of headaches.
> The source of the problem is that we do:
>
>   const char *some_var = "default";
>   ...
>   git_config_string(&some_var, ...);

Yup, that is a valid pattern for "run once and let exit(3) clean after us" programs.

Show 18 quoted lines

> Doing it "right" in C would probably involve two variables:
>
>   const char *some_var = "default";
>   const char *some_var_storage = NULL;
>
>   int git_config_string_smart(const char **ptr, char **storage,
>                               const char *var, const char *value)
>   {
>         ...
> 	free(*storage);
> 	*ptr = *storage = xstrdup(value);
> 	return 0;
>   }
>
>   #define GIT_CONFIG_STRING(name, var, value) \
>   git_config_string_smart(&(name), &(name##_storage), var, value)
>
> Or something like that.

The attitude the approach takes is that "run once and let exit(3) clean after us" programs *should* care. And at that point, maybe

	char *some_var = xstrdup("default");
	git_config_string(&some_var, ...);

that takes "char **" and frees the current storage before assigning to it may be simpler than the two-variable approach.

But you're right. We cannot just unconst the type and be done with it---there are associated clean-up necessary if we were to do this.

Thanks.