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

Hi Kaartic,

On Thu, 21 Dec 2017, Kaartic Sivaraam wrote:

Speaking of trace, (thanks to Dscho!) the one I got using 'valgrind'
(with `--leak-check=full` option) can be found below. I've kept only
relevant parts of it to avoid unwanted noise of `set -x`. Let me know
if that's needed too.

...

+ git diff-files --quiet --ignore-submodules
+ git diff-index --cached --quiet --ignore-submodules HEAD --
+ test 0 = 1
+ test -z 1
+ valgrind --leak-check=full git rebase--helper --continue
==10384== Memcheck, a memory error detector
==10384== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==10384== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==10384== Command: git rebase--helper --continue
==10384== 
==10384== Invalid free() / delete / delete[] / realloc()
==10384==    at 0x4C2CDDB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10384==    by 0x24E3F8: read_populate_opts (sequencer.c:1964)
==10384==    by 0x24E3F8: sequencer_continue (sequencer.c:2753)
==10384==    by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52)
==10384==    by 0x11B847: run_builtin (git.c:346)
==10384==    by 0x11B847: handle_builtin (git.c:554)
==10384==    by 0x11BB05: run_argv (git.c:606)
==10384==    by 0x11BB05: cmd_main (git.c:683)
==10384==    by 0x11AC0B: main (common-main.c:43)
==10384==  Address 0x2a898a is in a r-x mapped file /mnt/Source/Git/git-next/git segment
==10384== 
Successfully rebased and updated refs/heads/public.
==10384== 
==10384== HEAP SUMMARY:
==10384==     in use at exit: 680,882 bytes in 332 blocks
==10384==   total heap usage: 717 allocs, 386 frees, 1,709,293 bytes allocated
==10384== 
==10384== 2,053 (2,048 direct, 5 indirect) bytes in 1 blocks are definitely lost in loss record 75 of 83
==10384==    at 0x4C2BADF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10384==    by 0x4C2DE5F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10384==    by 0x27E0FE: xrealloc (wrapper.c:138)
==10384==    by 0x2072A3: add_object_array_with_path (object.c:319)
==10384==    by 0x23D745: add_pending_object_with_path (revision.c:163)
==10384==    by 0x24030E: add_pending_object_with_mode (revision.c:170)
==10384==    by 0x24030E: add_pending_object (revision.c:176)
==10384==    by 0x24030E: add_head_to_pending (revision.c:188)
==10384==    by 0x280EFA: has_uncommitted_changes.part.17 (wt-status.c:2288)
==10384==    by 0x24DF88: commit_staged_changes (sequencer.c:2705)
==10384==    by 0x24DF88: sequencer_continue (sequencer.c:2749)
==10384==    by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52)
==10384==    by 0x11B847: run_builtin (git.c:346)
==10384==    by 0x11B847: handle_builtin (git.c:554)
==10384==    by 0x11BB05: run_argv (git.c:606)
==10384==    by 0x11BB05: cmd_main (git.c:683)
==10384==    by 0x11AC0B: main (common-main.c:43)
==10384== 
==10384== LEAK SUMMARY:
==10384==    definitely lost: 2,048 bytes in 1 blocks
==10384==    indirectly lost: 5 bytes in 1 blocks
==10384==      possibly lost: 0 bytes in 0 blocks
==10384==    still reachable: 678,829 bytes in 330 blocks
==10384==         suppressed: 0 bytes in 0 blocks
==10384== Reachable blocks (those to which a pointer was found) are not shown.
==10384== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==10384== 
==10384== For counts of detected and suppressed errors, rerun with: -v
==10384== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
+ exit


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

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

But. There is a problem when you set commit.gpgSign = true (see
https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L162-L165):

	if (!strcmp(k, "commit.gpgsign")) {
		opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
		return 0;
	}

See how a "" is assigned to that field of type `char *`? It should not
even be possible, as the "" is implicitly read-only memory. And it
certainly was never malloc()ed.

So how to solve it? My suggestion would be to use xstrdup("") instead of
"" here.

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

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.

Ciao,
Dscho