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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux