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

From: Jeff King <peff@peff.net>
Date: Jun 4, 2018, 04:51 UTC
Message-ID: <20180604045122.GE14451@sigill.intra.peff.net>
In-Reply-To: <xmqqin6z5g8e.fsf@gitster-ct.c.googlers.com>

On Mon, Jun 04, 2018 at 01:26:57PM +0900, Junio C Hamano wrote:

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

Even with "let exit clean up", we are still leaking heap every time the variable is assigned after the first. Again, I don't think it matters that much in practice, but I think:

  [core]
  editor = foo
  editor = foo
  ...etc...

would leak arbitrary memory during the config parse, that would be allocated for the remainder of the program. I guess you could say exit() is handling it, but I think the point is that we are letting exit() handle memory that was potentially useful until we exit, not leaks. :)

Show 7 quoted lines

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

That _is_ much nicer, but you cannot use xstrdup() as the initializer for a global "static char *some_var", which is what the majority of the config variables are. It's this "static initializer sometimes, run-time heap sometimes" duality to the variables that makes handling it such a pain.

With that strategy, we'd have to have a big initialize_defaults() function. Which actually might not be _too_ bad since we now have common-main.c, but:

  - it sucks to keep the default values far away from the declarations

  - it does carry a runtime cost. Not a huge one, but it sucks to pay it
    on every program startup, even if we're not using those variables.

-Peff