Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)

On Friday 22 December 2017 05:19 PM, Johannes Schindelin wrote:

Hi Kaartic,

quoted

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