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

From: Jeff King <peff@peff.net>
Date: Jun 4, 2018, 05:00 UTC
Message-ID: <20180604050033.GA17818@sigill.intra.peff.net>
In-Reply-To: <20180604035637.GA15408@sigill.intra.peff.net>

On Sun, Jun 03, 2018 at 11:56:37PM -0400, Jeff King wrote:

Show 25 quoted lines

> So sometimes some_var needs to be freed and sometimes not (and every one
> of those uses is a potential leak, but it's OK because they're all
> program-lifetime globals anyway, and people don't _tend_ to set the same
> option over and over, leaking heap memory). And C being C, we can't
> convert a pointer-to-pointer-to-const into a pointer-to-pointer without
> an explicit cast.
> 
> 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.

Oh, one other option I forgot to mention: we already have an "intern" hashmap helper. So we could just replace the xstrdup() with strintern() and magically the memory isn't "leaked".

I think this is a little bit of a hack, though. It catches my:

  [core]
  editor = foo
  editor = foo

case, because we only ever make one copy of the string "foo". But if I do:

  [core]
  editor = 1
  editor = 2
  ...

then we get unique strings. And while they're not _technically_ leaked, since the hashmap still knows about them, they might as well be. They're still wasting space through the duration of the program run.

-Peff