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

From: Jeff King <peff@peff.net>
Date: Jun 4, 2018, 03:56 UTC
Message-ID: <20180604035637.GA15408@sigill.intra.peff.net>
In-Reply-To: <xmqqmuwb5i7k.fsf@gitster-ct.c.googlers.com>

On Mon, Jun 04, 2018 at 12:44:15PM +0900, Junio C Hamano wrote:

Show 22 quoted lines

> >> diff --git a/sequencer.c b/sequencer.c
> >> index b98690ecd41..aba03e9429a 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
> >>  			warning(_("invalid commit message cleanup mode '%s'"),
> >>  				  s);
> >>  
> >> +		free(s);
> >>  		return status;
> >>  	}
> >
> > Shouldn't 's' now lose 'const'?  After all, git_config_string()
> > gives you an allocated memory so...
> 
> Yikes.  Should git_config_string() and git_config_pathname() take
> "char **dst" instead of "const char **" as their out-location
> parameter?  They both assign a pointer to an allocated piece of
> memory for the caller to own or dispose of, but because of
> const-ness of the pointee their first parameter has, a caller like
> this one must declare "const char *s" yet is forced to call free()
> not to leak the value when it is done.

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, ...);

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.

We could also get away from an out-parameter and use the return type, since the single-pointer conversion is OK. But the primary value of git_config_string() is that it lets you return the error code as a one-liner.

-Peff