git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v10 33/36] merge-recursive: fix was_tracked() to quit lying with some renamed paths

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Apr 20, 2018, 12:23 UTC
Message-ID
<20180420122355.21416-1-szeder.dev@gmail.com>
In-Reply-To
<20180419175823.7946-34-newren@gmail.com>
Show 30 quoted lines
> In commit aacb82de3ff8 ("merge-recursive: Split was_tracked() out of
> would_lose_untracked()", 2011-08-11), was_tracked() was split out of
> would_lose_untracked() with the intent to provide a function that could
> answer whether a path was tracked in the index before the merge.  Sadly,
> it instead returned whether the path was in the working tree due to having
> been tracked in the index before the merge OR having been written there by
> unpack_trees().  The distinction is important when renames are involved,
> e.g. for a merge where:
> 
>    HEAD:  modifies path b
>    other: renames b->c
> 
> In this case, c was not tracked in the index before the merge, but would
> have been added to the index at stage 0 and written to the working tree by
> unpack_trees().  would_lose_untracked() is more interested in the
> in-working-copy-for-either-reason behavior, while all other uses of
> was_tracked() want just was-it-tracked-in-index-before-merge behavior.
> 
> Unsplit would_lose_untracked() and write a new was_tracked() function
> which answers whether a path was tracked in the index before the merge
> started.
> 
> This will also affect was_dirty(), helping it to return better results
> since it can base answers off the original index rather than an index that
> possibly only copied over some of the stat information.  However,
> was_dirty() will need an additional change that will be made in a
> subsequent patch.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

This patch causes memory corruption when the split index feature is in use, making several tests fail. Now, while the split index feature sure has its own set of problems, AFAIK those are not that bad to cause memory corruption, they "only" tend to cause transient test failures due to a variant of the classic racy git issue [1].

Here is a test failure:
  $ GIT_TEST_SPLIT_INDEX=DareISayYes ./t3030-merge-recursive.sh
  <...>
  ok 31 - merge-recursive simple w/submodule result
  *** Error in `/home/szeder/src/git/git': free(): invalid pointer: 0x0000000001f646d0 ***
  ======= Backtrace: =========
  /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f84e0c5b7e5]
  /lib/x86_64-linux-gnu/libc.so.6(+0x7f72a)[0x7f84e0c6372a]
  /lib/x86_64-linux-gnu/libc.so.6(cfree+0xf7)[0x7f84e0c685e7]
  /home/szeder/src/git/git[0x5181ee]
  /home/szeder/src/git/git[0x4f1e82]
  /home/szeder/src/git/git[0x4f394b]
  /home/szeder/src/git/git[0x44a37f]
  /home/szeder/src/git/git[0x44afa9]
  /home/szeder/src/git/git[0x406640]
  /home/szeder/src/git/git[0x4070f0]
  /home/szeder/src/git/git[0x4062a7]
  /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f84e0c04830]
  /home/szeder/src/git/git[0x4062f9]
  ======= Memory map: ========
  00400000-00616000 r-xp 00000000 08:06 2255502                            /home/szeder/src/git/git
  00815000-00816000 r--p 00215000 08:06 2255502                            /home/szeder/src/git/git
  00816000-00823000 rw-p 00216000 08:06 2255502                            /home/szeder/src/git/git
  00823000-00866000 rw-p 00000000 00:00 0 
  01f63000-01fa6000 rw-p 00000000 00:00 0                                  [heap]
  7f84e09ce000-7f84e09e4000 r-xp 00000000 08:06 921674                     /lib/x86_64-linux-gnu/libgcc_s.so.1
  7f84e09e4000-7f84e0be3000 ---p 00016000 08:06 921674                     /lib/x86_64-linux-gnu/libgcc_s.so.1
  7f84e0be3000-7f84e0be4000 rw-p 00015000 08:06 921674                     /lib/x86_64-linux-gnu/libgcc_s.so.1
  7f84e0be4000-7f84e0da4000 r-xp 00000000 08:06 917791                     /lib/x86_64-linux-gnu/libc-2.23.so
  7f84e0da4000-7f84e0fa4000 ---p 001c0000 08:06 917791                     /lib/x86_64-linux-gnu/libc-2.23.so
  7f84e0fa4000-7f84e0fa8000 r--p 001c0000 08:06 917791                     /lib/x86_64-linux-gnu/libc-2.23.so
  7f84e0fa8000-7f84e0faa000 rw-p 001c4000 08:06 917791                     /lib/x86_64-linux-gnu/libc-2.23.so
  7f84e0faa000-7f84e0fae000 rw-p 00000000 00:00 0 
  7f84e0fae000-7f84e0fb5000 r-xp 00000000 08:06 917825                     /lib/x86_64-linux-gnu/librt-2.23.so
  7f84e0fb5000-7f84e11b4000 ---p 00007000 08:06 917825                     /lib/x86_64-linux-gnu/librt-2.23.so
  7f84e11b4000-7f84e11b5000 r--p 00006000 08:06 917825                     /lib/x86_64-linux-gnu/librt-2.23.so
  7f84e11b5000-7f84e11b6000 rw-p 00007000 08:06 917825                     /lib/x86_64-linux-gnu/librt-2.23.so
  7f84e11b6000-7f84e11ce000 r-xp 00000000 08:06 917789                     /lib/x86_64-linux-gnu/libpthread-2.23.so
  7f84e11ce000-7f84e13cd000 ---p 00018000 08:06 917789                     /lib/x86_64-linux-gnu/libpthread-2.23.so
  7f84e13cd000-7f84e13ce000 r--p 00017000 08:06 917789                     /lib/x86_64-linux-gnu/libpthread-2.23.so
  7f84e13ce000-7f84e13cf000 rw-p 00018000 08:06 917789                     /lib/x86_64-linux-gnu/libpthread-2.23.so
  7f84e13cf000-7f84e13d3000 rw-p 00000000 00:00 0 
  7f84e13d3000-7f84e13ec000 r-xp 00000000 08:06 918601                     /lib/x86_64-linux-gnu/libz.so.1.2.8
  7f84e13ec000-7f84e15eb000 ---p 00019000 08:06 918601                     /lib/x86_64-linux-gnu/libz.so.1.2.8
  7f84e15eb000-7f84e15ec000 r--p 00018000 08:06 918601                     /lib/x86_64-linux-gnu/libz.so.1.2.8
  7f84e15ec000-7f84e15ed000 rw-p 00019000 08:06 918601                     /lib/x86_64-linux-gnu/libz.so.1.2.8
  7f84e15ed000-7f84e1613000 r-xp 00000000 08:06 917787                     /lib/x86_64-linux-gnu/ld-2.23.so
  7f84e1760000-7f84e17e5000 rw-p 00000000 00:00 0 
  7f84e1811000-7f84e1812000 rw-p 00000000 00:00 0 
  7f84e1812000-7f84e1813000 r--p 00025000 08:06 917787                     /lib/x86_64-linux-gnu/ld-2.23.so
  7f84e1813000-7f84e1814000 rw-p 00026000 08:06 917787                     /lib/x86_64-linux-gnu/ld-2.23.so
  7f84e1814000-7f84e1815000 rw-p 00000000 00:00 0 
  7ffff14d9000-7ffff14fa000 rw-p 00000000 00:00 0                          [stack]
  7ffff15cf000-7ffff15d2000 r--p 00000000 00:00 0                          [vvar]
  7ffff15d2000-7ffff15d4000 r-xp 00000000 00:00 0                          [vdso]
  ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
  not ok 32 - merge-recursive copy vs. rename
  #       
  #               git checkout -f copy &&
  #               git merge rename &&
  #               ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
  #               (
  #                       echo "100644 blob $o0   b"
  #                       echo "100644 blob $o0   c"
  #                       echo "100644 blob $o0   d/e"
  #                       echo "100644 blob $o0   e"
  #                       echo "100644 $o0 0      b"
  #                       echo "100644 $o0 0      c"
  #                       echo "100644 $o0 0      d/e"
  #                       echo "100644 $o0 0      e"
  #               ) >expected &&
  #               test_cmp expected actual
  #       
And the gdb backtrace of that 'git merge rename' command:
  Program received signal SIGABRT, Aborted.
  0x00007ffff7403428 in __GI_raise (sig=sig@entry=6)
      at ../sysdeps/unix/sysv/linux/raise.c:54
  54      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
  (gdb) bt
  #0  0x00007ffff7403428 in __GI_raise (sig=sig@entry=6)
      at ../sysdeps/unix/sysv/linux/raise.c:54
  #1  0x00007ffff740502a in __GI_abort () at abort.c:89
  #2  0x00007ffff74457ea in __libc_message (do_abort=do_abort@entry=2, 
      fmt=fmt@entry=0x7ffff755eed8 "*** Error in `%s': %s: 0x%s ***\n")
      at ../sysdeps/posix/libc_fatal.c:175
  #3  0x00007ffff744d72a in malloc_printerr (
      ar_ptr=0x7ffff7792b20 <main_arena>, ptr=<optimized out>, 
      str=0x7ffff755bcaf "free(): invalid pointer", action=<optimized out>)
      at malloc.c:5006
  #4  free_check (mem=<optimized out>, caller=<optimized out>) at hooks.c:314
  #5  0x00007ffff74525e7 in __GI___libc_free (mem=<optimized out>)
      at malloc.c:2942
  #6  0x00000000005181ee in discard_index (istate=istate@entry=0x7fffffffcc10)
      at read-cache.c:1934
  #7  0x00000000004f1e82 in merge_trees (o=o@entry=0x7fffffffc850, 
      head=<optimized out>, merge=<optimized out>, common=<optimized out>, 
      result=result@entry=0x7fffffffc7f0) at merge-recursive.c:3125
  #8  0x00000000004f394b in merge_recursive (o=o@entry=0x7fffffffc850, 
      h1=h1@entry=0x86efa0, h2=0x86f020, ca=0x0, 
      result=result@entry=0x7fffffffc840) at merge-recursive.c:3220
  #9  0x000000000044a37f in try_merge_strategy (strategy=<optimized out>, 
      strategy@entry=0x597ded "recursive", common=common@entry=0x8674c0, 
      remoteheads=remoteheads@entry=0x8673f0, head=head@entry=0x86efa0)
      at builtin/merge.c:690
  #10 0x000000000044afa9 in cmd_merge (argc=<optimized out>, 
      argv=<optimized out>, prefix=<optimized out>) at builtin/merge.c:1533
  #11 0x0000000000406640 in run_builtin (argv=<optimized out>, 
      argc=<optimized out>, p=<optimized out>) at git.c:350
  #12 handle_builtin (argc=2, argv=0x7fffffffdc30) at git.c:562
  #13 0x00000000004070f0 in run_argv (argv=0x7fffffffd9d0, argcp=0x7fffffffd9dc)
      at git.c:614
  #14 cmd_main (argc=2, argc@entry=3, argv=0x7fffffffdc30, 
      argv@entry=0x7fffffffdc28) at git.c:691
  #15 0x00000000004062a7 in main (argc=3, argv=0x7fffffffdc28)
      at common-main.c:45
Other failing tests are:
  t3030-merge-recursive.sh
  t3402-rebase-merge.sh
  t3501-revert-cherry-pick.sh
  t6022-merge-rename.sh
  t6032-merge-large-rename.sh
  t6034-merge-rename-nocruft.sh
  t6042-merge-rename-corner-cases.sh
  t6043-merge-rename-directories.sh
  t6046-merge-skip-unneeded-updates.sh
  t7003-filter-branch.sh
  t7601-merge-pull-config.sh
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b32e8d817a..097de7e5a7 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
Show 12 quoted lines
> @@ -3081,6 +3115,15 @@ int merge_trees(struct merge_options *o,
>  	else
>  		clean = 1;
>  
> +	/* Free the extra index left from git_merge_trees() */
> +	/*
> +	 * FIXME: Need to also data allocated by setup_unpack_trees_porcelain()
> +	 * tucked away in o->unpack_opts.msgs, but the problem is that only
> +	 * half of it refers to dynamically allocated data, while the other
> +	 * half points at static strings.
> +	 */
> +	discard_index(&o->orig_index);

Removing this discard_index() call makes all those test failures go away... but I guess that isn't the right solution, is it.

And even with that call removed, the next patch will cause a segmentation fault in 't6043-merge-rename-directories.sh's '72 - 9f-check: Renamed directory that only contained immediate subdirs'.

[1] Working on it: https://github.com/szeder/git split-index-racy
Previous: Elijah NewrenNext: Elijah Newren
Message 11 of 78 in “Add directory rename detection to git”
  1. 00/36 Add directory rename detection to gitElijah Newren, Apr 19, 2018
  2. 36/36 merge-recursive: fix check for skipability of working tree updatesElijah Newren, Apr 19, 2018
  3. 29/36 merge-recursive: improve add_cacheinfo error handlingElijah Newren, Apr 19, 2018
  4. 19/36 merge-recursive: check for directory level conflictsElijah Newren, Apr 19, 2018
  5. 07/36 directory rename detection: more involved edge/corner testcasesElijah Newren, Apr 19, 2018
  6. 26/36 merge-recursive: fix remaining directory rename + dirty overwrite casesElijah Newren, Apr 19, 2018
  7. 14/36 merge-recursive: fix leaks of allocated renames and diff_filepairsElijah Newren, Apr 19, 2018
  8. 33/36 merge-recursive: fix was_tracked() to quit lying with some renamed pathsElijah Newren, Apr 19, 2018
  9. Martin ÅgrenApr 19, 2018
  10. Elijah NewrenApr 19, 2018
  11. SZEDER GáborApr 20, 2018
  12. Elijah NewrenApr 20, 2018
  13. 5/36 unpack_trees: fix memory corruption with split_index when src != dstElijah Newren, Apr 21, 2018
  14. Elijah NewrenApr 21, 2018
  15. Duy NguyenApr 22, 2018
  16. Elijah NewrenApr 23, 2018
  17. Duy NguyenApr 23, 2018
  18. Elijah NewrenApr 23, 2018
  19. unpack_trees: fix breakage when o->src_index != o->dst_indexElijah Newren, Apr 24, 2018
  20. Junio C HamanoApr 24, 2018
  21. Junio C HamanoApr 24, 2018
  22. unpack_trees: fix breakage when o->src_index != o->dst_indexElijah Newren, Apr 24, 2018
  23. Duy NguyenApr 29, 2018
  24. Johannes SchindelinApr 29, 2018
  25. Duy NguyenApr 30, 2018
  26. Duy NguyenApr 30, 2018
  27. Elijah NewrenApr 30, 2018
  28. Duy NguyenApr 30, 2018
  29. 24/36 merge-recursive: avoid clobbering untracked files with directory renamesElijah Newren, Apr 19, 2018
  30. 34/36 merge-recursive: fix remainder of was_dirty() to use original indexElijah Newren, Apr 19, 2018
  31. 31/36 merge-recursive: avoid triggering add_cacheinfo error with dirty modElijah Newren, Apr 19, 2018
  32. 20/36 merge-recursive: add computation of collisions due to dir rename & mergingElijah Newren, Apr 19, 2018
  33. 32/36 t6046: testcases checking whether updates can be skipped in a mergeElijah Newren, Apr 19, 2018
  34. SZEDER GáborApr 19, 2018
  35. Elijah NewrenApr 19, 2018
  36. 35/36 merge-recursive: make "Auto-merging" comment show for other mergesElijah Newren, Apr 19, 2018
  37. 23/36 merge-recursive: apply necessary modifications for directory renamesElijah Newren, Apr 19, 2018
  38. 30/36 merge-recursive: move more is_dirty handling to merge_contentElijah Newren, Apr 19, 2018
  39. 28/36 merge-recursive: avoid spurious rename/rename conflict from dir renamesElijah Newren, Apr 19, 2018
  40. 18/36 merge-recursive: add get_directory_renames()Elijah Newren, Apr 19, 2018
  41. SZEDER GáborMay 6, 2018
  42. fixup! merge-recursive: add get_directory_renames()Elijah Newren, May 7, 2018
  43. Johannes SchindelinOct 9, 2019
  44. Elijah NewrenOct 11, 2019
  45. Johannes SchindelinOct 12, 2019
  46. 25/36 merge-recursive: fix overwriting dirty files involved in renamesElijah Newren, Apr 19, 2018
  47. Martin ÅgrenApr 19, 2018
  48. Martin ÅgrenApr 19, 2018
  49. Elijah NewrenApr 19, 2018
  50. 22/36 merge-recursive: when comparing files, don't include treesElijah Newren, Apr 19, 2018
  51. 11/36 directory rename detection: tests for handling overwriting dirty filesElijah Newren, Apr 19, 2018
  52. 06/36 directory rename detection: testcases checking which side did the renameElijah Newren, Apr 19, 2018
  53. 13/36 merge-recursive: introduce new functions to handle rename logicElijah Newren, Apr 19, 2018
  54. 10/36 directory rename detection: tests for handling overwriting untracked filesElijah Newren, Apr 19, 2018
  55. 21/36 merge-recursive: check for file level conflicts then get new nameElijah Newren, Apr 19, 2018
  56. 17/36 merge-recursive: make a helper function for cleanup for handle_renamesElijah Newren, Apr 19, 2018
  57. 16/36 merge-recursive: split out code for determining diff_filepairsElijah Newren, Apr 19, 2018
  58. 15/36 merge-recursive: make !o->detect_rename codepath more obviousElijah Newren, Apr 19, 2018
  59. 27/36 directory rename detection: new testcases showcasing a pair of bugsElijah Newren, Apr 19, 2018
  60. 01/36 directory rename detection: basic testcasesElijah Newren, Apr 19, 2018
  61. 08/36 directory rename detection: testcases exploring possibly suboptimal mergesElijah Newren, Apr 19, 2018
  62. 04/36 directory rename detection: partially renamed directory testcase/discussionElijah Newren, Apr 19, 2018
  63. 09/36 directory rename detection: miscellaneous testcases to complete coverageElijah Newren, Apr 19, 2018
  64. 02/36 directory rename detection: directory splitting testcasesElijah Newren, Apr 19, 2018
  65. 12/36 merge-recursive: move the get_renames() functionElijah Newren, Apr 19, 2018
  66. 03/36 directory rename detection: testcases to avoid taking detection too farElijah Newren, Apr 19, 2018
  67. 05/36 directory rename detection: files/directories in the way of some renamesElijah Newren, Apr 19, 2018
  68. Elijah NewrenApr 19, 2018
  69. Stefan BellerApr 19, 2018
  70. Derrick StoleeApr 19, 2018
  71. Elijah NewrenApr 19, 2018
  72. Junio C HamanoApr 20, 2018
  73. Elijah NewrenApr 23, 2018
  74. 1/2 fixup! merge-recursive: fix was_tracked() to quit lying with some renamed pathsElijah Newren, Apr 24, 2018
  75. 2/2 fixup! t6046: testcases checking whether updates can be skipped in a mergeElijah Newren, Apr 24, 2018
  76. Elijah NewrenApr 23, 2018
  77. Junio C HamanoApr 23, 2018
  78. Elijah NewrenApr 24, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.