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
