Re: [PATCH v3 7/7] run-command: fix detaching when running auto maintenance

> On Sat, Aug 17, 2024 at 08:14:24AM -0400, Jeff King wrote:
> > On Fri, Aug 16, 2024 at 12:45:17PM +0200, Patrick Steinhardt wrote:
> > 
> > > Fix this bug by asking git-gc(1) to not detach when it is being invoked
> > > via git-maintenance(1). Instead, git-maintenance(1) now respects a new
> > > config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and
> > > detaches itself into the background when running as part of our auto
> > > maintenance. This should continue to behave the same for all users which
> > > use the git-gc(1) task, only. For others though, it means that we now
> > > properly perform all tasks in the background. The default behaviour of
> > > git-maintenance(1) when executed by the user does not change, it will
> > > remain in the foreground unless they pass the `--detach` option.
> > 
> > This patch seems to cause segfaults in t5616 when combined with the
> > reftable backend. Try this:
> > 
> >   GIT_TEST_DEFAULT_REF_FORMAT=reftable ./t5616-partial-clone.sh --run=1-16 --stress
> > 
> > which fails for me within a few runs. Bisecting leads to 98077d06b2
> > (run-command: fix detaching when running auto maintenance, 2024-08-16).
> > It doesn't trigger with the files ref backend.
> > 
> > Compiling with ASan gets me a stack trace like this:
> > 
> >   + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin
> >   AddressSanitizer:DEADLYSIGNAL
> >   =================================================================
> >   ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)
> >   ==657994==The signal is caused by a READ memory access.
> >       #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29
> >       #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170
> >       #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194
> >       #3 0x55f23e54e72e in block_iter_next reftable/block.c:398
> >       #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240
> >       #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355
> >       #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339
> >       #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69
> >       #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123
> >       #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172
> >       #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175
> >       #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464
> >       #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13
> >       #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452
> >       #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623
> >       #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659
> >       #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133
> >       #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432
> >       #18 0x55f23dba7764 in run_builtin git.c:484
> >       #19 0x55f23dba7764 in handle_builtin git.c:741
> >       #20 0x55f23dbab61e in run_argv git.c:805
> >       #21 0x55f23dbab61e in cmd_main git.c:1000
> >       #22 0x55f23dba4781 in main common-main.c:64
> >       #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> >       #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360
> >       #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)
> 
> I haven't yet been able to definitely tell, but I think this is a
> lifetime issue. We create an iterator, eventually notice that the
> reftable stack has been rewritten, and reload the stack. But the
> block sources used for the old tables are still referenced by the
> iterator, even though it was closed. As such, the mmapped memory of the
> table has been unmapped and is now invalid, which causes the above
> invalid reads.
> 
> I'll work on a patch series that introduces refcounting for block
> sources, but guess that'll take a bit.