From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Mon, 25 Dec 2017 08:51:39 GMT
Subject: Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
Message-ID: <71f9236c-9e55-8489-54b2-5d7c7df1eb0c@gmail.com>
In-Reply-To: <alpine.DEB.2.21.1.1712221201210.406@MININT-6BKU6QN.europe.corp.microsoft.com>

On Friday 22 December 2017 05:19 PM, Johannes Schindelin wrote:
> Hi Kaartic,
> 
>>
>> I think I didn't mention I've set `commit.gpgsign` to `true` for that
>> repo, did I?
> 
> Hah! I had troubles to associate the correct line in my versions of Git's
> source code (the line numbers alone are only reliable if you also have a
> commit from which the Git binaries were built), 

Should have mentioned that, sorry.


> but I did this free() at
> (https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L1975:
> 
> 	if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> 		if (!starts_with(buf.buf, "-S"))
> 			strbuf_reset(&buf);
> 		else {
> 			free(opts->gpg_sign);
> 			^^^^^^^^^^^^^^^^^^^^^
> 			opts->gpg_sign = xstrdup(buf.buf + 2);
> 		}
> 		strbuf_reset(&buf);
> 	}
> 

Seems you got the right one, regardless. :)


> The culprit is that we have these really unclear ownership rules, where it
> is not at all clear who is responsible for releasing allocated memory:
> caller or callee? (Hannes would not rightfully point out that this would
> be a non-issue if we would switch to C++). In C, the common pattern is to
> use `const char *` for users that are *not* supposed to take ownership,
> and `char *` for users that are supposed to take custody. And in this
> instance, `gpg_sign` should be owned by `struct replay_opts` because of
> this (https://github.com/git/git/blob/cd54ea2b18/sequencer.h#L38):
> 
> 	char *gpg_sign;
> 
> Technically, using `char *` (instead of `const char *`) does not say
> "you're now responsible for de-allocating this memory", of course, but in
> practice it is often used like this (and the signature of `free(void *)`
> certainly encourages that type of interpreting the `const` qualifier).
> 

Nice explanation.


> I spent a little quality time with the code and came up with a patch that
> I will send out in a moment.
>

That relieves Philip of the burden, I guess. :)


> By the way, Kaartic, thank you so much for focusing on correctness before
> focusing on style issues. I know it is harder to review patches for
> correctness than it is to concentrate on style issues, but in my mind it
> is not only much more work, but also a lot more valuable.
> 

Though it's good to hear these words and I do like to focus on 
correctness, there wasn't much I did to focus on correctness in this 
case ;-) It was you actually, after seeing such a clear explanation!.

I just used Git built from 'next' for my personal use and encountered 
the issue I stated in the start of this sub-thread.


-- 
Kaartic