mbox series

[00/11] rebase: reset_head() related fixes and improvements

Message ID pull.1049.git.1633082702.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series rebase: reset_head() related fixes and improvements | expand

Message

Derrick Stolee via GitGitGadget Oct. 1, 2021, 10:04 a.m. UTC 
Fix some issues with the implementation and use of reset_head(). The last
patch was previously posted as [1], I have updated the commit message and
rebased it onto the fixes in this series. There are a couple of small
conflicts merging this into seen, I think they should be easy to resolve (in
rebase.c take both sides in reset.c take the changed lines from each side).
These patches are based on pw/rebase-of-a-tag-fix

[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/

Phillip Wood (11):
  rebase: factor out checkout for up to date branch
  reset_head(): fix checkout
  reset_head(): don't run checkout hook if there is an error
  reset_head(): remove action parameter
  reset_head(): factor out ref updates
  reset_head(): make default_reflog_action optional
  rebase: cleanup reset_head() calls
  reset_head(): take struct rebase_head_opts
  rebase --apply: fix reflog
  rebase --apply: set ORIG_HEAD correctly
  rebase -m: don't fork git checkout

 builtin/merge.c            |   6 +-
 builtin/rebase.c           |  97 +++++++++++++++----------
 reset.c                    | 143 ++++++++++++++++++++++---------------
 reset.h                    |  33 +++++++--
 sequencer.c                |  48 ++++---------
 sequencer.h                |   3 +-
 t/t3406-rebase-message.sh  |  23 ++++++
 t/t3418-rebase-continue.sh |  26 +++++++
 8 files changed, 240 insertions(+), 139 deletions(-)


base-commit: 7740ac691d8e7f1bed67bcbdb1ee5c5c618f7373
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1049

Comments

Junio C Hamano Oct. 2, 2021, 12:38 a.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Fix some issues with the implementation and use of reset_head(). The last
> patch was previously posted as [1], I have updated the commit message and
> rebased it onto the fixes in this series. There are a couple of small
> conflicts merging this into seen, I think they should be easy to resolve (in
> rebase.c take both sides in reset.c take the changed lines from each side).
> These patches are based on pw/rebase-of-a-tag-fix

When merged with other topics in flight in 'seen', this seems to
fail the t1092 test (most likely, ds/add-rm-with-sparse-index is
what this interacts badly with).

Thanks.
Junio C Hamano Oct. 2, 2021, 4:58 a.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> When merged with other topics in flight in 'seen', this seems to
> fail the t1092 test (most likely, ds/add-rm-with-sparse-index is
> what this interacts badly with).

There are Two CI runs at GitHub:

 - https://github.com/git/git/actions/runs/1297117791 (d3a1c4e)
 - https://github.com/git/git/actions/runs/1297232973 (a1108c2)

The difference between the former (which passes all the tests) and
he latter (which fails) are only two topics:

    $ git log --first-parent --oneline d3a1c4e..a1108c2
    a1108c2b1b Merge branch 'hn/reftable' into seen
    e575f29006 Merge branch 'pw/fix-some-issues-in-reset-head' into seen

I think the following is the same failure I saw earlier

   https://github.com/git/git/runs/3773780195?check_suite_focus=true#step:6:5033

that the write-tree codepath hits assertion failure by detecting a
corruption in the cache-tree data structure.

e575f29006 (i.e. without the reftable topic) fails t1092.  If you
revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from
that merge, all tests pass including t1092.

The reftable topic is queued near the tip of 'seen' not necessarily
because it _breaks_ CI (I do not think it does), but it needed a
handful of fixup commits on top.  The topic needs rerolling with the
fixes squashed in.

Thanks.
Phillip Wood Oct. 2, 2021, 12:27 p.m. UTC | #3 
Hi Junio

On 02/10/2021 05:58, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> When merged with other topics in flight in 'seen', this seems to
>> fail the t1092 test (most likely, ds/add-rm-with-sparse-index is
>> what this interacts badly with).

Oh dear, thanks for letting me know.

> There are Two CI runs at GitHub:
> 
>   - https://github.com/git/git/actions/runs/1297117791 (d3a1c4e)
>   - https://github.com/git/git/actions/runs/1297232973 (a1108c2)
> 
> The difference between the former (which passes all the tests) and
> he latter (which fails) are only two topics:
> 
>      $ git log --first-parent --oneline d3a1c4e..a1108c2
>      a1108c2b1b Merge branch 'hn/reftable' into seen
>      e575f29006 Merge branch 'pw/fix-some-issues-in-reset-head' into seen
> 
> I think the following is the same failure I saw earlier
> 
>     https://github.com/git/git/runs/3773780195?check_suite_focus=true#step:6:5033
> 
> that the write-tree codepath hits assertion failure by detecting a
> corruption in the cache-tree data structure.

The test that fails (t1092-sparse-checkout-compatibility.sh:'merge, 
cherry-pick, and rebase') was introduced by c0b99303db ("t1092: add 
cherry-pick, rebase tests", 2021-09-08) and is in v2.33.0. It does not 
test the "apply" backend of rebase, changing it to do so makes it fail 
on the current master as that backend already uses reset_head() for the 
initial checkout so it is an existing bug that is exposed by the changes 
in this series. It seems to be a use-after-free (see below) I'll try and 
investigate but I've got no idea what is going on at the moment - the 
index is not my area of expertise.

Best Wishes

Phillip

==74345==ERROR: AddressSanitizer: heap-use-after-free on address 
0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080
READ of size 4 at 0x606000001b20 thread T0
     #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
     #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #4 0x557cbe830a2b in cache_tree_verify 
/home/phil/src/git/cache-tree.c:910
     #5 0x557cbea53741 in write_locked_index 
/home/phil/src/git/read-cache.c:3250
     #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
     #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
     #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
     #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
     #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
     #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
     #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
     #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
     #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)

0x606000001b20 is located 0 bytes inside of 56-byte region 
[0x606000001b20,0x606000001b58)
freed by thread T0 here:
     #0 0x7fdd4bacff19 in __interceptor_free 
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
     #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35
     #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
     #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
     #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
     #5 0x557cbeb2557a in ensure_full_index 
/home/phil/src/git/sparse-index.c:310
     #6 0x557cbea45c4a in index_name_stage_pos 
/home/phil/src/git/read-cache.c:588
     #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
     #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
     #11 0x557cbe830a2b in cache_tree_verify 
/home/phil/src/git/cache-tree.c:910
     #12 0x557cbea53741 in write_locked_index 
/home/phil/src/git/read-cache.c:3250
     #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
     #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
     #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
     #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
     #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
     #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
     #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
     #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

previously allocated by thread T0 here:
     #0 0x7fdd4bad0459 in __interceptor_calloc 
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
     #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
     #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
     #3 0x557cbe82b7d8 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:763
     #4 0x557cbe82b837 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:764
     #5 0x557cbe82b837 in prime_cache_tree_rec 
/home/phil/src/git/cache-tree.c:764
     #6 0x557cbe8304e1 in prime_cache_tree 
/home/phil/src/git/cache-tree.c:779
     #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
     #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
     #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
     #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
     #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
     #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
     #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
     #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

SUMMARY: AddressSanitizer: heap-use-after-free 
/home/phil/src/git/cache-tree.c:863 in verify_one
Shadow bytes around the buggy address:
   0x0c0c7fff8310: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
   0x0c0c7fff8320: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
   0x0c0c7fff8330: fa fa fa fa 00 00 00 00 00 00 00 02 fa fa fa fa
   0x0c0c7fff8340: fd fd fd fd fd fd fd fa fa fa fa fa 00 00 00 00
   0x0c0c7fff8350: 00 00 00 02 fa fa fa fa fd fd fd fd fd fd fd fa
=>0x0c0c7fff8360: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
   0x0c0c7fff8370: 00 00 00 00 00 00 00 02 fa fa fa fa fd fd fd fd
   0x0c0c7fff8380: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 02
   0x0c0c7fff8390: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
   0x0c0c7fff83a0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
   0x0c0c7fff83b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
   Addressable:           00
   Partially addressable: 01 02 03 04 05 06 07
   Heap left redzone:       fa
   Freed heap region:       fd
   Stack left redzone:      f1
   Stack mid redzone:       f2
   Stack right redzone:     f3
   Stack after return:      f5
   Stack use after scope:   f8
   Global redzone:          f9
   Global init order:       f6
   Poisoned by user:        f7
   Container overflow:      fc
   Array cookie:            ac
   Intra object redzone:    bb
   ASan internal:           fe
   Left alloca redzone:     ca
   Right alloca redzone:    cb
   Shadow gap:              cc
==74345==ABORTING


> e575f29006 (i.e. without the reftable topic) fails t1092.  If you
> revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from
> that merge, all tests pass including t1092.
> 
> The reftable topic is queued near the tip of 'seen' not necessarily
> because it _breaks_ CI (I do not think it does), but it needed a
> handful of fixup commits on top.  The topic needs rerolling with the
> fixes squashed in.
> 
> Thanks.
>
Phillip Wood Oct. 2, 2021, 1:12 p.m. UTC | #4 
On 02/10/2021 13:27, Phillip Wood wrote:
> Hi Junio
> 
> On 02/10/2021 05:58, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> When merged with other topics in flight in 'seen', this seems to
>>> fail the t1092 test (most likely, ds/add-rm-with-sparse-index is
>>> what this interacts badly with).
> 
> Oh dear, thanks for letting me know.
> 
>> There are Two CI runs at GitHub:
>>
>>   - https://github.com/git/git/actions/runs/1297117791 (d3a1c4e)
>>   - https://github.com/git/git/actions/runs/1297232973 (a1108c2)
>>
>> The difference between the former (which passes all the tests) and
>> he latter (which fails) are only two topics:
>>
>>      $ git log --first-parent --oneline d3a1c4e..a1108c2
>>      a1108c2b1b Merge branch 'hn/reftable' into seen
>>      e575f29006 Merge branch 'pw/fix-some-issues-in-reset-head' into seen
>>
>> I think the following is the same failure I saw earlier
>>
>>     
>> https://github.com/git/git/runs/3773780195?check_suite_focus=true#step:6:5033 
>>
>>
>> that the write-tree codepath hits assertion failure by detecting a
>> corruption in the cache-tree data structure.
> 
> The test that fails (t1092-sparse-checkout-compatibility.sh:'merge, 
> cherry-pick, and rebase') was introduced by c0b99303db ("t1092: add 
> cherry-pick, rebase tests", 2021-09-08) and is in v2.33.0. It does not 
> test the "apply" backend of rebase, changing it to do so makes it fail 
> on the current master as that backend already uses reset_head() for the 
> initial checkout so it is an existing bug that is exposed by the changes 
> in this series. It seems to be a use-after-free (see below) I'll try and 
> investigate but I've got no idea what is going on at the moment - the 
> index is not my area of expertise.

removing the call to prime_cache_tree() from reset_head() fixes the 
crash. It is called after unpack_trees() and before 
write_locked_index(), I'm not sure what the implications of removing it 
are - why it was there it is needed?

Best Wishes

Phillip

> Best Wishes
> 
> Phillip
> 
> ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 
> 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080
> READ of size 4 at 0x606000001b20 thread T0
>      #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
>      #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #4 0x557cbe830a2b in cache_tree_verify 
> /home/phil/src/git/cache-tree.c:910
>      #5 0x557cbea53741 in write_locked_index 
> /home/phil/src/git/read-cache.c:3250
>      #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>      #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>      #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>      #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>      #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>      #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>      #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>      #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>      #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)
> 
> 0x606000001b20 is located 0 bytes inside of 56-byte region 
> [0x606000001b20,0x606000001b58)
> freed by thread T0 here:
>      #0 0x7fdd4bacff19 in __interceptor_free 
> /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
>      #1 0x557cbe82af60 in cache_tree_free 
> /home/phil/src/git/cache-tree.c:35
>      #2 0x557cbe82aee5 in cache_tree_free 
> /home/phil/src/git/cache-tree.c:31
>      #3 0x557cbe82aee5 in cache_tree_free 
> /home/phil/src/git/cache-tree.c:31
>      #4 0x557cbe82aee5 in cache_tree_free 
> /home/phil/src/git/cache-tree.c:31
>      #5 0x557cbeb2557a in ensure_full_index 
> /home/phil/src/git/sparse-index.c:310
>      #6 0x557cbea45c4a in index_name_stage_pos 
> /home/phil/src/git/read-cache.c:588
>      #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
>      #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>      #11 0x557cbe830a2b in cache_tree_verify 
> /home/phil/src/git/cache-tree.c:910
>      #12 0x557cbea53741 in write_locked_index 
> /home/phil/src/git/read-cache.c:3250
>      #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>      #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>      #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>      #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>      #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>      #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>      #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>      #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
> 
> previously allocated by thread T0 here:
>      #0 0x7fdd4bad0459 in __interceptor_calloc 
> /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
>      #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
>      #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
>      #3 0x557cbe82b7d8 in prime_cache_tree_rec 
> /home/phil/src/git/cache-tree.c:763
>      #4 0x557cbe82b837 in prime_cache_tree_rec 
> /home/phil/src/git/cache-tree.c:764
>      #5 0x557cbe82b837 in prime_cache_tree_rec 
> /home/phil/src/git/cache-tree.c:764
>      #6 0x557cbe8304e1 in prime_cache_tree 
> /home/phil/src/git/cache-tree.c:779
>      #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
>      #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>      #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>      #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>      #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>      #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>      #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>      #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
> 
> SUMMARY: AddressSanitizer: heap-use-after-free 
> /home/phil/src/git/cache-tree.c:863 in verify_one
> Shadow bytes around the buggy address:
>    0x0c0c7fff8310: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>    0x0c0c7fff8320: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
>    0x0c0c7fff8330: fa fa fa fa 00 00 00 00 00 00 00 02 fa fa fa fa
>    0x0c0c7fff8340: fd fd fd fd fd fd fd fa fa fa fa fa 00 00 00 00
>    0x0c0c7fff8350: 00 00 00 02 fa fa fa fa fd fd fd fd fd fd fd fa
> =>0x0c0c7fff8360: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
>    0x0c0c7fff8370: 00 00 00 00 00 00 00 02 fa fa fa fa fd fd fd fd
>    0x0c0c7fff8380: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 02
>    0x0c0c7fff8390: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
>    0x0c0c7fff83a0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>    0x0c0c7fff83b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>    Addressable:           00
>    Partially addressable: 01 02 03 04 05 06 07
>    Heap left redzone:       fa
>    Freed heap region:       fd
>    Stack left redzone:      f1
>    Stack mid redzone:       f2
>    Stack right redzone:     f3
>    Stack after return:      f5
>    Stack use after scope:   f8
>    Global redzone:          f9
>    Global init order:       f6
>    Poisoned by user:        f7
>    Container overflow:      fc
>    Array cookie:            ac
>    Intra object redzone:    bb
>    ASan internal:           fe
>    Left alloca redzone:     ca
>    Right alloca redzone:    cb
>    Shadow gap:              cc
> ==74345==ABORTING
> 
> 
>> e575f29006 (i.e. without the reftable topic) fails t1092.  If you
>> revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from
>> that merge, all tests pass including t1092.
>>
>> The reftable topic is queued near the tip of 'seen' not necessarily
>> because it _breaks_ CI (I do not think it does), but it needed a
>> handful of fixup commits on top.  The topic needs rerolling with the
>> fixes squashed in.
>>
>> Thanks.
>>
>
René Scharfe Oct. 2, 2021, 1:38 p.m. UTC | #5 
Am 02.10.21 um 14:27 schrieb Phillip Wood:
> Hi Junio
>
> On 02/10/2021 05:58, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> When merged with other topics in flight in 'seen', this seems to
>>> fail the t1092 test (most likely, ds/add-rm-with-sparse-index is
>>> what this interacts badly with).
>
> Oh dear, thanks for letting me know.
>
>> There are Two CI runs at GitHub:
>>
>>   - https://github.com/git/git/actions/runs/1297117791 (d3a1c4e)
>>   - https://github.com/git/git/actions/runs/1297232973 (a1108c2)
>>
>> The difference between the former (which passes all the tests) and
>> he latter (which fails) are only two topics:
>>
>>      $ git log --first-parent --oneline d3a1c4e..a1108c2
>>      a1108c2b1b Merge branch 'hn/reftable' into seen
>>      e575f29006 Merge branch 'pw/fix-some-issues-in-reset-head' into seen
>>
>> I think the following is the same failure I saw earlier
>>
>>     https://github.com/git/git/runs/3773780195?check_suite_focus=true#step:6:5033
>>
>> that the write-tree codepath hits assertion failure by detecting a
>> corruption in the cache-tree data structure.
>
> The test that fails (t1092-sparse-checkout-compatibility.sh:'merge, cherry-pick, and rebase') was introduced by c0b99303db ("t1092: add cherry-pick, rebase tests", 2021-09-08) and is in v2.33.0. It does not test the "apply" backend of rebase, changing it to do so makes it fail on the current master as that backend already uses reset_head() for the initial checkout so it is an existing bug that is exposed by the changes in this series. It seems to be a use-after-free (see below) I'll try and investigate but I've got no idea what is going on at the moment - the index is not my area of expertise.
>
> Best Wishes
>
> Phillip
>
> ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080
> READ of size 4 at 0x606000001b20 thread T0
>     #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
>     #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910
>     #5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
>     #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>     #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>     #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>     #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>     #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>     #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>     #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>     #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>     #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)
>
> 0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58)
> freed by thread T0 here:
>     #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
>     #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35
>     #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>     #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>     #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>     #5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310
>     #6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588
>     #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
>     #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>     #11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910

Looks like cache_tree_verify() indirectly triggered ensure_full_index(),
which throws the cache_tree away, then kept going using stale pointers.
It should instead stop and start over in such a case, no?

>     #12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
>     #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>     #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>     #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>     #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>     #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>     #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>     #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>     #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>
> previously allocated by thread T0 here:
>     #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
>     #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
>     #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
>     #3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763
>     #4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
>     #5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
>     #6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779
>     #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
>     #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>     #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>     #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>     #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>     #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>     #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>     #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>
> SUMMARY: AddressSanitizer: heap-use-after-free /home/phil/src/git/cache-tree.c:863 in verify_one
> Shadow bytes around the buggy address:
>   0x0c0c7fff8310: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>   0x0c0c7fff8320: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
>   0x0c0c7fff8330: fa fa fa fa 00 00 00 00 00 00 00 02 fa fa fa fa
>   0x0c0c7fff8340: fd fd fd fd fd fd fd fa fa fa fa fa 00 00 00 00
>   0x0c0c7fff8350: 00 00 00 02 fa fa fa fa fd fd fd fd fd fd fd fa
> =>0x0c0c7fff8360: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
>   0x0c0c7fff8370: 00 00 00 00 00 00 00 02 fa fa fa fa fd fd fd fd
>   0x0c0c7fff8380: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 02
>   0x0c0c7fff8390: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
>   0x0c0c7fff83a0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>   0x0c0c7fff83b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
>   Shadow gap:              cc
> ==74345==ABORTING
>
>
>> e575f29006 (i.e. without the reftable topic) fails t1092.  If you
>> revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from
>> that merge, all tests pass including t1092.
>>
>> The reftable topic is queued near the tip of 'seen' not necessarily
>> because it _breaks_ CI (I do not think it does), but it needed a
>> handful of fixup commits on top.  The topic needs rerolling with the
>> fixes squashed in.
>>
>> Thanks.
>>
>
Phillip Wood Oct. 6, 2021, 2:03 p.m. UTC | #6 
Hi René

On 02/10/2021 14:38, René Scharfe wrote:
>[..]
>> ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080
>> READ of size 4 at 0x606000001b20 thread T0
>>      #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
>>      #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910
>>      #5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
>>      #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>>      #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>>      #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>>      #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>>      #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>>      #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>>      #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>>      #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>>      #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)
>>
>> 0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58)
>> freed by thread T0 here:
>>      #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
>>      #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35
>>      #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>>      #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>>      #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
>>      #5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310
>>      #6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588
>>      #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
>>      #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
>>      #11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910
> 
> Looks like cache_tree_verify() indirectly triggered ensure_full_index(),
> which throws the cache_tree away, then kept going using stale pointers.
> It should instead stop and start over in such a case, no?

Thanks for that, I found it really helpful

Best Wishes

Phillip

>>      #12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
>>      #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
>>      #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>>      #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>>      #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>>      #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>>      #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>>      #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>>      #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>>
>> previously allocated by thread T0 here:
>>      #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
>>      #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
>>      #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
>>      #3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763
>>      #4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
>>      #5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
>>      #6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779
>>      #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
>>      #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
>>      #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
>>      #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
>>      #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
>>      #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
>>      #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
>>      #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
>>
>> SUMMARY: AddressSanitizer: heap-use-after-free /home/phil/src/git/cache-tree.c:863 in verify_one
>> Shadow bytes around the buggy address:
>>    0x0c0c7fff8310: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>>    0x0c0c7fff8320: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
>>    0x0c0c7fff8330: fa fa fa fa 00 00 00 00 00 00 00 02 fa fa fa fa
>>    0x0c0c7fff8340: fd fd fd fd fd fd fd fa fa fa fa fa 00 00 00 00
>>    0x0c0c7fff8350: 00 00 00 02 fa fa fa fa fd fd fd fd fd fd fd fa
>> =>0x0c0c7fff8360: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
>>    0x0c0c7fff8370: 00 00 00 00 00 00 00 02 fa fa fa fa fd fd fd fd
>>    0x0c0c7fff8380: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 02
>>    0x0c0c7fff8390: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
>>    0x0c0c7fff83a0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
>>    0x0c0c7fff83b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
>> Shadow byte legend (one shadow byte represents 8 application bytes):
>>    Addressable:           00
>>    Partially addressable: 01 02 03 04 05 06 07
>>    Heap left redzone:       fa
>>    Freed heap region:       fd
>>    Stack left redzone:      f1
>>    Stack mid redzone:       f2
>>    Stack right redzone:     f3
>>    Stack after return:      f5
>>    Stack use after scope:   f8
>>    Global redzone:          f9
>>    Global init order:       f6
>>    Poisoned by user:        f7
>>    Container overflow:      fc
>>    Array cookie:            ac
>>    Intra object redzone:    bb
>>    ASan internal:           fe
>>    Left alloca redzone:     ca
>>    Right alloca redzone:    cb
>>    Shadow gap:              cc
>> ==74345==ABORTING
>>
>>
>>> e575f29006 (i.e. without the reftable topic) fails t1092.  If you
>>> revert e575f29006^2 (i.e. the "do not fork 'git checkout'") from
>>> that merge, all tests pass including t1092.
>>>
>>> The reftable topic is queued near the tip of 'seen' not necessarily
>>> because it _breaks_ CI (I do not think it does), but it needed a
>>> handful of fixup commits on top.  The topic needs rerolling with the
>>> fixes squashed in.
>>>
>>> Thanks.
>>>
>>
>