Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable expensive tests #6

Closed
wants to merge 2 commits into from

Conversation

t-b
Copy link

@t-b t-b commented Oct 6, 2014

It also includes a backport of a fix for t0027 from junio/master.
All tests pass.

tboegi and others added 2 commits October 6, 2014 19:18
Add test cases for core.eol "native" and "" (unset).
(MINGW uses CRLF, all other systems LF as native line endings)

Add test cases for the attributes "eol=lf" and "eol=crlf"

Other minor changes:
- Use the more portable 'tr' instead of 'od -c' to convert '\n' into 'Q'
  and '\0' into 'N'
- Style fixes for shell functions according to the coding guide lines
- Replace "txtbin" with "attr"

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This increases the test count from 11623 to 11929.
And running time (Core i7-3930K@3.2GHz) from 18 minutes to 24 minutes with
10 parallel jobs.

Signed-off-by: Thomas Braun <thomas.braun@byte-physics.de>
@t-b
Copy link
Author

t-b commented Oct 6, 2014

@sschuberth I added some info into the commit message. I did not feel any heavier than without expensive tests cpu usage.

@dscho My intentition is to increase test coverage. I also don't like the concept of expensive tests, as everything which deviates from default setup is likely not tested. This is also the reason why I propose to change that in the source code level, rather on the test invocation level. With the expensive tests enabled, also every contributor will execute them, if he is doing that locally.

@dscho
Copy link
Member

dscho commented Oct 7, 2014

With the expensive tests enabled, also every contributor will execute them, if he is doing that locally.

We really have to be pragmatic here. If running tests takes an unbearable time, nobody will test them locally. So I would vote to let Jenkins do the heavy tests and let the default be at least somewhat bearable.

@sschuberth
Copy link

I tend to agree with Dscho. Also, a jump from 18 to 24 minutes test time (on your high end machine) is quite big. I'd expect the VM to take much longer, and it's already taking too long for my taste. Finally, it seems we'd be the only platform enabling expensive tests by default, so that's probably not a good idea.

@t-b
Copy link
Author

t-b commented Oct 7, 2014

I just checked all expensive tests separately.
Only t3302-notes-index-expensive.sh is really expensive with 6 minutes(!!). The other expensive tests finish within 50 seconds.

From the expensive tests I care mostly about t0027 because:

  • it has a huge MINGW only section
  • EOL fun is especially common on MINGW

t0027 takes here only 40 seconds.

So how about instead of enabling the EXPENSIVE tests, we remove the EXPENSIVE flag from t0027?
The test suite will not take noticibly longer to execute, but we have some important tests more?

@dscho
Copy link
Member

dscho commented Oct 7, 2014

I just checked all expensive tests separately.
Only t3302-notes-index-expensive.sh is really expensive with 6 minutes(!!). The other expensive tests finish within 50 seconds.

You could make an argument that some of the tests marked "expensive" are not expensive. That still does not change the fact that we should not force the expensive tests down the throat of contributors. Because if we tried that stunt, we'd only make sure that no contributor would run the tests at all...

So how about simply adding a Jenkins job that runs the expensive tests once a day, risk that regressions in those expensive tests are identified only with an up-to-24h delay, have happier contributors and quicker turn-arounds from BuildHive/Jenkins and be done with this issue?

@t-b
Copy link
Author

t-b commented Oct 7, 2014

So how about simply adding a Jenkins job that runs the expensive tests once a day, risk that regressions in those expensive tests are identified only with an up-to-24h delay, have happier contributors and quicker turn-arounds from BuildHive/Jenkins and be done with this issue?

Fine with me. But then we also should cherry-pick t-b@4f355f1.

@sschuberth
Copy link

So how about instead of enabling the EXPENSIVE tests, we remove the EXPENSIVE flag from t0027?

I like that idea. If the test is not (very) expensive, there is no need to flag it as such, and I'd like to run these MinGW-specific test by default.

@dscho
Copy link
Member

dscho commented Oct 8, 2014

How about dscho@t0027-mingw?

@dscho
Copy link
Member

dscho commented Oct 8, 2014

we also should cherry-pick t-b/git@4f355f1.

Agreed!

@sschuberth
Copy link

Dscho, when you push dscho@t0027-mingw, could you also take care or cherry-picking t-b@4f355f1? Thanks in advance! I'll close this PR then.

@sschuberth sschuberth closed this Oct 8, 2014
@dscho
Copy link
Member

dscho commented Oct 8, 2014

See #7.

dscho pushed a commit that referenced this pull request Mar 6, 2015
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@t-b t-b deleted the enable_expensive_tests branch May 5, 2015 21:26
dscho pushed a commit that referenced this pull request Jul 14, 2017
We turn off ASan's leak detection by default in the test
suite because it's too noisy. But we don't do so until
part-way through test-lib. This is before we've run any
tests, but after we do our initial "./git" to see if the
binary has even been built.

When built with clang, this seems to work fine. However,
using "gcc -fsanitize=address", the leak checker seems to
complain more aggressively:

  $ ./git
  ...
  ==5352==ERROR: LeakSanitizer: detected memory leaks
  Direct leak of 2 byte(s) in 1 object(s) allocated from:
      #0 0x7f120e7afcf8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1cf8)
      #1 0x559fc2a3ce41 in do_xmalloc /home/peff/compile/git/wrapper.c:60
      #2 0x559fc2a3cf1a in do_xmallocz /home/peff/compile/git/wrapper.c:100
      #3 0x559fc2a3d0ad in xmallocz /home/peff/compile/git/wrapper.c:108
      #4 0x559fc2a3d0ad in xmemdupz /home/peff/compile/git/wrapper.c:124
      #5 0x559fc2a3d0ad in xstrndup /home/peff/compile/git/wrapper.c:130
      #6 0x559fc274535a in main /home/peff/compile/git/common-main.c:39
      #7 0x7f120dabd2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

This is a leak in the sense that we never free it, but it's
in a global that is meant to last the whole program. So it's
not really interesting or in need of fixing. And at any
rate, mentioning leaks outside of the test_expect blocks is
certainly unwelcome, as it pollutes stderr.

Let's bump the setting of ASAN_OPTIONS higher in test-lib.sh
to catch our initial "can we even run git?" test.  While
we're at it, we can add a comment to make it a bit less
inscrutable.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Sep 8, 2017
This is to address concerns raised by ThreadSanitizer on the mailing list
about threaded unprotected R/W access to map.size with my previous "disallow
rehash" change (0607e10).

See:
https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/

Add API to hashmap to disable item counting and thus automatic rehashing.
Also include API to later re-enable them.

When item counting is disabled, the map.size field is invalid.  So to
prevent accidents, the field has been renamed and an accessor function
hashmap_get_size() has been added.  All direct references to this
field have been been updated.  And the name of the field changed
to map.private_size to communicate this.

Here is the relevant output from ThreadSanitizer showing the problem:

WARNING: ThreadSanitizer: data race (pid=10554)
  Read of size 4 at 0x00000082d488 by thread T2 (mutexes: write M16):
    #0 hashmap_add hashmap.c:209
    #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302
    #2 handle_range_dir name-hash.c:347
    #3 handle_range_1 name-hash.c:415
    #4 lazy_dir_thread_proc name-hash.c:471
    #5 <null> <null>

  Previous write of size 4 at 0x00000082d488 by thread T1 (mutexes: write M31):
    #0 hashmap_add hashmap.c:209
    #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302
    #2 handle_range_dir name-hash.c:347
    #3 handle_range_1 name-hash.c:415
    #4 handle_range_dir name-hash.c:380
    #5 handle_range_1 name-hash.c:415
    #6 lazy_dir_thread_proc name-hash.c:471
    #7 <null> <null>

Martin gives instructions for running TSan on test t3008 in this post:
https://public-inbox.org/git/CAN0heSoJDL9pWELD6ciLTmWf-a=oyxe4EXXOmCKvsG5MSuzxsA@mail.gmail.com/

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
jamill pushed a commit to jamill/git that referenced this pull request Jul 26, 2018
…e-status-rename

GVFS serialize status updates
jamill pushed a commit to jamill/git that referenced this pull request Nov 20, 2018
…e-status-rename

GVFS serialize status updates
garimasi514 referenced this pull request in garimasi514/git Aug 13, 2019
dscho pushed a commit that referenced this pull request Feb 24, 2021
write_commit_graph initialises topo_levels using init_topo_level_slab(),
next it calls compute_topological_levels() which can cause the slab to
grow, we therefore need to clear the slab again using
clear_topo_level_slab() when we're done.

First introduced in 72a2bfc (commit-graph: add a slab to store
topological levels, 2021-01-16).

LeakSanitizer output:

==1026==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x498ae9 in realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0xafbed8 in xrealloc /src/git/wrapper.c:126:8
    #2 0x7966d1 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
    #10 0x4cd084 in run_argv /src/git/git.c:771:4
    #11 0x4ca424 in cmd_main /src/git/git.c:902:19
    #12 0x707fb6 in main /src/git/common-main.c:52:11
    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

Indirect leak of 524256 byte(s) in 1 object(s) allocated from:
    #0 0x498942 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0xafc088 in xcalloc /src/git/wrapper.c:140:8
    #2 0x796870 in topo_level_slab_at_peek /src/git/commit-graph.c:71:1
    #3 0x7965e0 in topo_level_slab_at /src/git/commit-graph.c:71:1
    #4 0x78fbf5 in compute_topological_levels /src/git/commit-graph.c:1472:12
    #5 0x78c5c3 in write_commit_graph /src/git/commit-graph.c:2456:2
    #6 0x535c5f in graph_write /src/git/builtin/commit-graph.c:299:6
    #7 0x5350ca in cmd_commit_graph /src/git/builtin/commit-graph.c:337:11
    #8 0x4cddb1 in run_builtin /src/git/git.c:453:11
    #9 0x4cabe2 in handle_builtin /src/git/git.c:704:3
    #10 0x4cd084 in run_argv /src/git/git.c:771:4
    #11 0x4ca424 in cmd_main /src/git/git.c:902:19
    #12 0x707fb6 in main /src/git/common-main.c:52:11
    #13 0x7fee4249383f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)

SUMMARY: AddressSanitizer: 524264 byte(s) leaked in 2 allocation(s).

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 10, 2021
This leak has existed since:
9ab55da (git symbolic-ref --delete $symref, 2012-10-21)

This leak was found when running t0001 with LSAN, see also LSAN output
below:

Direct leak of 19 byte(s) in 1 object(s) allocated from:
    #0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
    #2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
    #3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
    #4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
    #5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 10, 2021
dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard
it, we can FREE_AND_NULL.

This leak appears to have been introduced in:
4cf76f6 (builtin/reset: compute checkout metadata for reset, 2020-03-16)

This leak was found when running t0001 with LSAN, see also LSAN output below:

Direct leak of 5 byte(s) in 1 object(s) allocated from:
    #0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0x9a7108 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
    #2 0x8add6b in expand_ref /home/ahunt/oss-fuzz/git/refs.c:670:12
    #3 0x8ad777 in repo_dwim_ref /home/ahunt/oss-fuzz/git/refs.c:644:22
    #4 0x6394af in dwim_ref /home/ahunt/oss-fuzz/git/./refs.h:162:9
    #5 0x637e5c in cmd_reset /home/ahunt/oss-fuzz/git/builtin/reset.c:426:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c5ce in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f57ebb9d349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 10, 2021
Most of these pointers can safely be freed when cmd_clone() completes,
therefore we make sure to free them. The one exception is that we
have to UNLEAK(repo) because it can point either to argv[0], or a
malloc'd string returned by absolute_pathdup().

We also have to free(path) in the middle of cmd_clone(): later during
cmd_clone(), path is unconditionally overwritten with a different path,
triggering a leak. Freeing the first path immediately after use (but
only in the case where it contains data) seems like the cleanest
solution, as opposed to freeing it unconditionally before path is reused
for another path. This leak appears to have been introduced in:
  f38aa83 (use local cloning if insteadOf makes a local URL, 2014-07-17)

These leaks were found when running t0001 with LSAN, see also an excerpt
of the LSAN output below (the full list is omitted because it's far too
long, and mostly consists of indirect leakage of members of the refs we
are freeing).

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21
    #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
vv    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f8fef0c2349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a6b2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8
    #2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20
    #3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9
    #4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8
    #5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8
    #6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4
    #7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9
    #8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4
    #9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9
    #10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #15 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a6b2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8
    #2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20
    #3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9
    #4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8
    #5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8
    #6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4
    #7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9
    #8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4
    #9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9
    #10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #15 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 105 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9a71f6 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x93622d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x937a73 in strbuf_addch /home/ahunt/oss-fuzz/git/./strbuf.h:231:3
    #4 0x939fcd in strbuf_add_absolute_path /home/ahunt/oss-fuzz/git/strbuf.c:911:4
    #5 0x69d3ce in absolute_pathdup /home/ahunt/oss-fuzz/git/abspath.c:261:2
    #6 0x51c688 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1021:10
    #7 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #8 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #9 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #10 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #11 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #12 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 10, 2021
Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).

This leak appears to have been introduced in:
  f60a7b7 (worktree: teach "add" to check out existing branches, 2018-04-24)

Note that UNLEAK(branchname) is still needed: the returned result is
used in add(), and is stored in a pointer which is used to point at one
of:
  - a string literal ("HEAD")
  - member of argv (whatever the user specified in their invocation)
  - or our newly allocated string returned from dwim_branch()
Fixing the branchname leak isn't impossible, but does not seem
worthwhile given that add() is called directly from cmd_main(), and
cmd_main() returns immediately thereafter - UNLEAK is good enough.

This leak was found when running t0001 with LSAN, see also LSAN output
below:

Direct leak of 60 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3
    #4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2
    #5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20
    #6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19
    #7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10
    #8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #13 0x7f7b7dd10349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 10, 2021
The primary goal of this change is to stop leaking init_db_template_dir.
This leak can happen because:
 1. git_init_db_config() allocates new memory into init_db_template_dir
    without first freeing the existing value.
 2. init_db_template_dir might already contain data, either because:
  2.1 git_config() can be invoked twice with this callback in a single
      process - at least 2 allocations are likely.
  2.2 A single git_config() allocation can invoke the callback multiple
      times for a given key (see further explanation in the function
      docs) - each of those calls will trigger another leak.

The simplest fix for the leak would be to free(init_db_template_dir)
before overwriting it. Instead we choose to convert to fetching
init.templatedir via git_config_get_value() as that is more explicit,
more efficient, and avoids allocations (the returned result is owned by
the config cache, so we aren't responsible for freeing it).

If we remove init_db_template_dir, git_init_db_config() ends up being
responsible only for forwarding core.* config values to
platform_core_config(). However platform_core_config() already ignores
non-core.* config values, so we can safely remove git_init_db_config()
and invoke git_config() directly with platform_core_config() as the
callback.

The platform_core_config forwarding was originally added in:
  2878533 (mingw: respect core.hidedotfiles = false in git-init again, 2019-03-11
And I suspect the potential for a leak existed since the original
implementation of git_init_db_config in:
  90b4518 (Add `init.templatedir` configuration variable., 2010-02-17)

LSAN output from t0001:

Direct leak of 73 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9a7276 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x9362ad in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x936eaa in strbuf_add /home/ahunt/oss-fuzz/git/strbuf.c:295:2
    #4 0x868112 in strbuf_addstr /home/ahunt/oss-fuzz/git/./strbuf.h:304:2
    #5 0x86a8ad in expand_user_path /home/ahunt/oss-fuzz/git/path.c:758:2
    #6 0x720bb1 in git_config_pathname /home/ahunt/oss-fuzz/git/config.c:1287:10
    #7 0x5960e2 in git_init_db_config /home/ahunt/oss-fuzz/git/builtin/init-db.c:161:11
    #8 0x7255b8 in configset_iter /home/ahunt/oss-fuzz/git/config.c:1982:7
    #9 0x7253fc in repo_config /home/ahunt/oss-fuzz/git/config.c:2311:2
    #10 0x725ca7 in git_config /home/ahunt/oss-fuzz/git/config.c:2399:2
    #11 0x593e8d in create_default_files /home/ahunt/oss-fuzz/git/builtin/init-db.c:225:2
    #12 0x5935c6 in init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:449:11
    #13 0x59588e in cmd_init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:714:9
    #14 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #15 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #16 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #17 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #18 0x69c4de in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #19 0x7f23552d6349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Feb 21, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Feb 22, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 1, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Mar 30, 2022
In the preceding [1] (pack-objects: move revs out of
get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
to cmd_pack_objects() so that it unconditionally took place for all
invocations of "git pack-objects".

We'd thus start leaking memory, which is easily reproduced in
e.g. git.git by feeding e83c516 (Initial revision of "git", the
information manager from hell, 2005-04-07) to "git pack-objects";

    $ echo e83c516 | ./git pack-objects initial
    [...]
	==19130==ERROR: LeakSanitizer: detected memory leaks

	Direct leak of 7120 byte(s) in 1 object(s) allocated from:
	    #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)
	    #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8
	    #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9
	    #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2
	    #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2
	    #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2
	    #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2
	    #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11
	    #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3
	    #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4
	    #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19
	    #11 0x562259 in main /home/avar/g/git/common-main.c:56:11
	    #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16
	    #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)

	SUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).
	Aborted

Narrowly fixing that commit would have been easy, just add call
repo_init_revisions() right before get_object_list(), which is
effectively what was done before that commit.

But an unstated constraint when setting it up early is that it was
needed for the subsequent [2] (pack-objects: parse --filter directly
into revs.filter, 2022-03-22), i.e. we might have a --filter
command-line option, and need to either have the "struct rev_info"
setup when we encounter that option, or later.

Let's just change the control flow so that we'll instead set up the
"struct rev_info" only when we need it. Doing so leads to a bit more
verbosity, but it's a lot clearer what we're doing and why.

An earlier version of this commit[3] went behind
opt_parse_list_objects_filter()'s back by faking up a "struct option"
before calling it. Let's avoid that and instead create a blessed API
for this pattern.

We could furthermore combine the two get_object_list() invocations
here by having repo_init_revisions() invoked on &pfd.revs, but I think
clearly separating the two makes the flow clearer. Likewise
redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
"have_revs" early in cmd_pack_objects().

While we're at it add parentheses around the arguments to the OPT_*
macros in in list-objects-filter-options.h, as we need to change those
lines anyway. It doesn't matter in this case, but is good general
practice.

1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com
2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com
3. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
mjcheetham pushed a commit to mjcheetham/git that referenced this pull request Jun 16, 2022
git-for-windows-ci pushed a commit that referenced this pull request Aug 15, 2022
Since commit fcc07e9 (is_promisor_object(): free tree buffer after
parsing, 2021-04-13), we'll always free the buffers attached to a
"struct tree" after searching them for promisor links. But there's an
important case where we don't want to do so: if somebody else is already
using the tree!

This can happen during a "rev-list --missing=allow-promisor" traversal
in a partial clone that is missing one or more trees or blobs. The
backtrace for the free looks like this:

      #1 free_tree_buffer tree.c:147
      #2 add_promisor_object packfile.c:2250
      #3 for_each_object_in_pack packfile.c:2190
      #4 for_each_packed_object packfile.c:2215
      #5 is_promisor_object packfile.c:2272
      #6 finish_object__ma builtin/rev-list.c:245
      #7 finish_object builtin/rev-list.c:261
      #8 show_object builtin/rev-list.c:274
      #9 process_blob list-objects.c:63
      #10 process_tree_contents list-objects.c:145
      #11 process_tree list-objects.c:201
      #12 traverse_trees_and_blobs list-objects.c:344
      [...]

We're in the middle of walking through the entries of a tree object via
process_tree_contents(). We see a blob (or it could even be another tree
entry) that we don't have, so we call is_promisor_object() to check it.
That function loops over all of the objects in the promisor packfile,
including the tree we're currently walking. When we're done with it
there, we free the tree buffer. But as we return to the walk in
process_tree_contents(), it's still holding on to a pointer to that
buffer, via its tree_desc iterator, and it accesses the freed memory.

Even a trivial use of "--missing=allow-promisor" triggers this problem,
as the included test demonstrates (it's just a vanilla --blob:none
clone).

We can detect this case by only freeing the tree buffer if it was
allocated on our behalf. This is a little tricky since that happens
inside parse_object(), and it doesn't tell us whether the object was
already parsed, or whether it allocated the buffer itself. But by
checking for an already-parsed tree beforehand, we can distinguish the
two cases.

That feels a little hacky, and does incur an extra lookup in the
object-hash table. But that cost is fairly minimal compared to actually
loading objects (and since we're iterating the whole pack here, we're
likely to be loading most objects, rather than reusing cached results).

It may also be a good direction for this function in general, as there
are other possible optimizations that rely on doing some analysis before
parsing:

  - we could detect blobs and avoid reading their contents; they can't
    link to other objects, but parse_object() doesn't know that we don't
    care about checking their hashes.

  - we could avoid allocating object structs entirely for most objects
    (since we really only need them in the oidset), which would save
    some memory.

  - promisor commits could use the commit-graph rather than loading the
    object from disk

This commit doesn't do any of those optimizations, but I think it argues
that this direction is reasonable, rather than relying on parse_object()
and trying to teach it to give us more information about whether it
parsed.

The included test fails reliably under SANITIZE=address just when
running "rev-list --missing=allow-promisor". Checking the output isn't
strictly necessary to detect the bug, but it seems like a reasonable
addition given the general lack of coverage for "allow-promisor" in the
test suite.

Reported-by: Andrew Olsen <andrew.olsen@koordinates.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-for-windows-ci pushed a commit that referenced this pull request Aug 22, 2022
Fix a memory leak occuring in case of pathspec copy in preload_index.

Direct leak of 8 byte(s) in 8 object(s) allocated from:
    #0 0x7f0a353ead47 in __interceptor_malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/libasan.so.6+0xb5d47)
    #1 0x55750995e840 in do_xmalloc /home/anthony/src/c/git/wrapper.c:51
    #2 0x55750995e840 in xmalloc /home/anthony/src/c/git/wrapper.c:72
    #3 0x55750970f824 in copy_pathspec /home/anthony/src/c/git/pathspec.c:684
    #4 0x557509717278 in preload_index /home/anthony/src/c/git/preload-index.c:135
    #5 0x55750975f21e in refresh_index /home/anthony/src/c/git/read-cache.c:1633
    #6 0x55750915b926 in cmd_status builtin/commit.c:1547
    #7 0x5575090e1680 in run_builtin /home/anthony/src/c/git/git.c:466
    #8 0x5575090e1680 in handle_builtin /home/anthony/src/c/git/git.c:720
    #9 0x5575090e284a in run_argv /home/anthony/src/c/git/git.c:787
    #10 0x5575090e284a in cmd_main /home/anthony/src/c/git/git.c:920
    #11 0x5575090dbf82 in main /home/anthony/src/c/git/common-main.c:56
    #12 0x7f0a348230ab  (/lib64/libc.so.6+0x290ab)

Signed-off-by: Anthony Delannoy <anthony.2lannoy@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Oct 30, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:

#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56

Backtrace from the death is:

#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Oct 31, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
Add config option `windows.appendAtomically`

Atomic append on windows is only supported on local disk files, and it may
cause errors in other situations, e.g. network file system. If that is the
case, this config option should be used to turn atomic append off.

With these edits, status for old-style submodules with commondir
needs to be fixed, due to the following.

In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Co-Authored-By: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: 孙卓识 <sunzhuoshi@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
sceptical-coder added a commit to sceptical-coder/git that referenced this pull request Nov 3, 2022
Add config option `windows.appendAtomically`

Atomic append on windows is only supported on local disk files, and it may
cause errors in other situations, e.g. network file system. If that is the
case, this config option should be used to turn atomic append off.

With these edits, the status command for old-style submodules with commondir
needs to be fixed, due to the following.

In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, after the addition of the new config option, when
`git status` is run in the root repo of such a setup, it gives an output
akin to this:
```sh
$ git status
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
$ GIT_DIR=.git git -C commonlibs/ status --porcelain=2
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:
```
#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
    at compat/mingw.c:784
git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#8  0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```
Backtrace from the death is:
```
#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
    at usage.c:210
git-for-windows#1  0x<address-41> in access_or_die (
    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
    at wrapper.c:667
git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
    at config.c:2142
git-for-windows#3  0x<address-38> in config_with_options (
    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
    config_source=0x0, opts=0x<address-35>) at config.c:2198
git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
    at config.c:2524
git-for-windows#5  0x<address-33> in git_config_check_init (
    repo=0x<address-19> <the_repo>) at config.c:2543
git-for-windows#6  0x<address-32> in repo_config_get_bool (
    repo=0x<address-19> <the_repo>,
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2612
git-for-windows#7  0x<address-31> in git_config_get_bool (
    key=0x<address-30> <pad+3116> "windows.appendatomically",
    dest=0x<address-29> <append_atomically>) at config.c:2714
git-for-windows#8  0x<address-28> in mingw_open (
    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
    gitdir=0x<address-22> ".git") at setup.c:313
git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
    commondir=0x0) at repository.c:57
git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
    at environment.c:179
git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
    at environment.c:334
git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
git-for-windows#16 0x<address-12> in chdir_notify (
    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
    argc=2, argv=0x<address-2>) at git.c:458
git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
    at git.c:721
git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
    at git.c:788
git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
    at common-main.c:56
```

Co-Authored-By: Johannes Schindelin <johannes.schindelin@gmx.de>
Co-Authored-By: Andrey Zabavnikov <zabavnikov@gmail.com>
Signed-off-by: 孙卓识 <sunzhuoshi@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
dscho pushed a commit to sceptical-coder/git that referenced this pull request Nov 4, 2022
In some setups, old-style submodules (i.e. the ones
with .git directory within theirs worktrees) with commondir
can be of tremendous help. For example, commondir link can be
used to avoid duplication of objects and also to keep branches
in sync with multiple copies of the repo's worktree, while keeping
the .git directory inside the worktree can be (ab?-)used to exploit
the sharing of the same submodule worktree across different projects
(this at least works on Windows with submodule directory being
a directory junction, but having a junction is not relevant for
reproducing the bug described below).

Unfortunately, at the moment, when `git status` is run in the root repo
of such a setup, it gives an output akin to this:
```sh
fatal: unable to access '�??\1?/config': Invalid argument
fatal: 'git status --porcelain=2' failed in submodule commonlibs
```
where `�??\1?` part of '�??\1?/config' varies from run to run, and
`commonlibs` is the name of submodule's directory.

Currently, when Git discovers old-style submodule , it spawns subprocess
to get its status, like this one:
```sh
cd commonlibs; unset GIT_PREFIX; GIT_DIR=.git git status --porcelain=2
```
Unsurprisingly, the following output is also quite unexpected:
```
fatal: unable to access '`??L&?/config': Invalid argument
```

The core reason for these is that global repository field for
commondir is not being cleared to `NULL` after being `free()`'d
in `repo_set_commondir()`, which is precisely what this commit fixes.

Regarding the further details of the case of investigation,
this value of struct pointed by the global `the_repository` pointer is
checked for being not-NULL down in the callstack in compatibility layer
for MinGW in a function that is called by `repo_set_commondir()` before
the `free()`'d value gets assigned in its body (i.e. the body of
`repo_set_commondir()`).

Backtrace from the check is:

	#0  mingw_open (filename=0x<address-25> ".git/commondir", oflags=0)
	    at compat/mingw.c:784
	git-for-windows#1  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
	    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
	git-for-windows#2  0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
	    gitdir=0x<address-22> ".git") at setup.c:313
	git-for-windows#3  0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
	    commondir=0x0) at repository.c:57
	git-for-windows#4  0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
	    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
	git-for-windows#5  0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
	    at environment.c:179
	git-for-windows#6  0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
	    at environment.c:334
	git-for-windows#7  0x<address-14> in update_relative_gitdir (name=0x0,
	    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
	    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
	git-for-windows#8  0x<address-12> in chdir_notify (
	    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
	git-for-windows#9  0x<address-10> in setup_work_tree () at setup.c:428
	git-for-windows#10 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
	    argc=2, argv=0x<address-2>) at git.c:458
	git-for-windows#11 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
	    at git.c:721
	git-for-windows#12 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
	    at git.c:788
	git-for-windows#13 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
	git-for-windows#14 0x<address-1> in main (argc=6, argv=0x<address-0>)
	    at common-main.c:56

Backtrace from the death is:

	#0  die_errno (fmt=0x<address-42> <result_type+2002> "unable to access '%s'")
	    at usage.c:210
	git-for-windows#1  0x<address-41> in access_or_die (
	    path=0x<address-40> "`\001\r��\004/config", mode=4, flag=0)
	    at wrapper.c:667
	git-for-windows#2  0x<address-39> in do_git_config_sequence (opts=0x<address-35>,
	    fn=0x<address-37> <git_config_include>, data=0x<address-36>)
	    at config.c:2142
	git-for-windows#3  0x<address-38> in config_with_options (
	    fn=0x<address-37> <git_config_include>, data=0x<address-36>,
	    config_source=0x0, opts=0x<address-35>) at config.c:2198
	git-for-windows#4  0x<address-34> in repo_read_config (repo=0x<address-19> <the_repo>)
	    at config.c:2524
	git-for-windows#5  0x<address-33> in git_config_check_init (
	    repo=0x<address-19> <the_repo>) at config.c:2543
	git-for-windows#6  0x<address-32> in repo_config_get_bool (
	    repo=0x<address-19> <the_repo>,
	    key=0x<address-30> <pad+3116> "windows.appendatomically",
	    dest=0x<address-29> <append_atomically>) at config.c:2612
	git-for-windows#7  0x<address-31> in git_config_get_bool (
	    key=0x<address-30> <pad+3116> "windows.appendatomically",
	    dest=0x<address-29> <append_atomically>) at config.c:2714
	git-for-windows#8  0x<address-28> in mingw_open (
	    filename=0x<address-25> ".git/commondir", oflags=0) at compat/mingw.c:785
	git-for-windows#9  0x<address-27> in strbuf_read_file (sb=0x<address-26>,
	    path=0x<address-25> ".git/commondir", hint=0) at strbuf.c:758
	git-for-windows#10 0x<address-24> in get_common_dir_noenv (sb=0x<address-23>,
	    gitdir=0x<address-22> ".git") at setup.c:313
	git-for-windows#11 0x<address-21> in repo_set_commondir (repo=0x<address-19> <the_repo>,
	    commondir=0x0) at repository.c:57
	git-for-windows#12 0x<address-20> in repo_set_gitdir (repo=0x<address-19> <the_repo>,
	    root=0x<address-15> ".git", o=0x<address-18>) at repository.c:76
	git-for-windows#13 0x<address-17> in setup_git_env (git_dir=0x<address-15> ".git")
	    at environment.c:179
	git-for-windows#14 0x<address-16> in set_git_dir_1 (path=0x<address-15> ".git")
	    at environment.c:334
	git-for-windows#15 0x<address-14> in update_relative_gitdir (name=0x0,
	    old_cwd=0x<address-13> "C:/Users/%username%/<root-repo-name>/commonlibs",
	    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs", data=0x0) at environment.c:348
	git-for-windows#16 0x<address-12> in chdir_notify (
	    new_cwd=0x<address-11> "C:/Users/%username%/<root-repo-name>/commonlibs") at chdir-notify.c:72
	git-for-windows#17 0x<address-10> in setup_work_tree () at setup.c:428
	git-for-windows#18 0x<address-9> in run_builtin (p=0x<address-8> <commands+2856>,
	    argc=2, argv=0x<address-2>) at git.c:458
	git-for-windows#19 0x<address-7> in handle_builtin (argc=2, argv=0x<address-2>)
	    at git.c:721
	git-for-windows#20 0x<address-6> in run_argv (argcp=0x<address-5>, argv=0x<address-4>)
	    at git.c:788
	git-for-windows#21 0x<address-3> in cmd_main (argc=2, argv=0x<address-2>) at git.c:921
	git-for-windows#22 0x<address-1> in main (argc=6, argv=0x<address-0>)
	    at common-main.c:56

Signed-off-by: Andrey Zabavnikov <zabavnikov@gmail.com>
dscho pushed a commit that referenced this pull request Nov 9, 2022
When "read_strategy_opts()" is called we may have populated the
"opts->strategy" before, so we'll need to free() it to avoid leaking
memory.

We populate it before because we cal get_replay_opts() from within
"rebase.c" with an already populated "opts", which we then copy. Then
if we're doing a "rebase -i" the sequencer API itself will promptly
clobber our alloc'd version of it with its own.

If this code is changed to do, instead of the added free() here a:

	if (opts->strategy)
		opts->strategy = xstrdup("another leak");

We get a couple of stacktraces from -fsanitize=leak showing how we
ended up clobbering the already allocated value, i.e.:

	Direct leak of 6 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x66bcb8 in read_strategy_opts sequencer.c:2902
	    #4 0x66bf7b in read_populate_opts sequencer.c:2969
	    #5 0x6723f9 in sequencer_continue sequencer.c:5063
	    #6 0x4a4f74 in run_sequencer_rebase builtin/rebase.c:348
	    #7 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #8 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #9 0x407a32 in run_builtin git.c:466
	    #10 0x407e0a in handle_builtin git.c:721
	    #11 0x40803d in run_argv git.c:788
	    #12 0x40850f in cmd_main git.c:923
	    #13 0x4eee79 in main common-main.c:57
	    #14 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #15 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #16 0x405fd0 in _start (git+0x405fd0)

	Direct leak of 4 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x4a3c31 in xstrdup_or_null git-compat-util.h:1169
	    #4 0x4a447a in get_replay_opts builtin/rebase.c:163
	    #5 0x4a4f5b in run_sequencer_rebase builtin/rebase.c:346
	    #6 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #7 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #8 0x407a32 in run_builtin git.c:466
	    #9 0x407e0a in handle_builtin git.c:721
	    #10 0x40803d in run_argv git.c:788
	    #11 0x40850f in cmd_main git.c:923
	    #12 0x4eee79 in main common-main.c:57
	    #13 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #14 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #15 0x405fd0 in _start (git+0x405fd0)

This can be seen in e.g. the 4th test of
"t3404-rebase-interactive.sh".

In the larger picture the ownership of the "struct replay_opts" is
quite a mess, e.g. in this case rebase.c's static "get_replay_opts()"
function partially creates it, but nothing in rebase.c will free()
it. The structure is "mostly owned" by the sequencer API, but it also
expects to get these partially populated versions of it.

It would be better to have rebase keep track of what it allocated, and
free() that, and to pass that as a "const" to the sequencer API, which
would copy what it needs to its own version, and to free() that.

But doing so is a much larger change, and however messy the ownership
boundary is here is consistent with what we're doing already, so let's
just free() this to fix the leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
dscho pushed a commit that referenced this pull request Nov 24, 2022
When "read_strategy_opts()" is called we may have populated the
"opts->strategy" before, so we'll need to free() it to avoid leaking
memory.

We populate it before because we cal get_replay_opts() from within
"rebase.c" with an already populated "opts", which we then copy. Then
if we're doing a "rebase -i" the sequencer API itself will promptly
clobber our alloc'd version of it with its own.

If this code is changed to do, instead of the added free() here a:

	if (opts->strategy)
		opts->strategy = xstrdup("another leak");

We get a couple of stacktraces from -fsanitize=leak showing how we
ended up clobbering the already allocated value, i.e.:

	Direct leak of 6 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x66bcb8 in read_strategy_opts sequencer.c:2902
	    #4 0x66bf7b in read_populate_opts sequencer.c:2969
	    #5 0x6723f9 in sequencer_continue sequencer.c:5063
	    #6 0x4a4f74 in run_sequencer_rebase builtin/rebase.c:348
	    #7 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #8 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #9 0x407a32 in run_builtin git.c:466
	    #10 0x407e0a in handle_builtin git.c:721
	    #11 0x40803d in run_argv git.c:788
	    #12 0x40850f in cmd_main git.c:923
	    #13 0x4eee79 in main common-main.c:57
	    #14 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #15 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #16 0x405fd0 in _start (git+0x405fd0)

	Direct leak of 4 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x4a3c31 in xstrdup_or_null git-compat-util.h:1169
	    #4 0x4a447a in get_replay_opts builtin/rebase.c:163
	    #5 0x4a4f5b in run_sequencer_rebase builtin/rebase.c:346
	    #6 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #7 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #8 0x407a32 in run_builtin git.c:466
	    #9 0x407e0a in handle_builtin git.c:721
	    #10 0x40803d in run_argv git.c:788
	    #11 0x40850f in cmd_main git.c:923
	    #12 0x4eee79 in main common-main.c:57
	    #13 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #14 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #15 0x405fd0 in _start (git+0x405fd0)

This can be seen in e.g. the 4th test of
"t3404-rebase-interactive.sh".

In the larger picture the ownership of the "struct replay_opts" is
quite a mess, e.g. in this case rebase.c's static "get_replay_opts()"
function partially creates it, but nothing in rebase.c will free()
it. The structure is "mostly owned" by the sequencer API, but it also
expects to get these partially populated versions of it.

It would be better to have rebase keep track of what it allocated, and
free() that, and to pass that as a "const" to the sequencer API, which
would copy what it needs to its own version, and to free() that.

But doing so is a much larger change, and however messy the ownership
boundary is here is consistent with what we're doing already, so let's
just free() this to fix the leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
There is an out-of-bounds read possible when parsing gitattributes that
have an attribute that is 2^31+1 bytes long. This is caused due to an
integer overflow when we assign the result of strlen(3P) to an `int`,
where we use the wrapped-around value in a subsequent call to
memcpy(3P). The following code reproduces the issue:

    blob=$(perl -e 'print "a" x 2147483649 . " attr"' | git hash-object -w --stdin)
    git update-index --add --cacheinfo 100644,$blob,.gitattributes
    git check-attr --all file

    AddressSanitizer:DEADLYSIGNAL
    =================================================================
    ==8451==ERROR: AddressSanitizer: SEGV on unknown address 0x7f93efa00800 (pc 0x7f94f1f8f082 bp 0x7ffddb59b3a0 sp 0x7ffddb59ab28 T0)
    ==8451==The signal is caused by a READ memory access.
        #0 0x7f94f1f8f082  (/usr/lib/libc.so.6+0x176082)
        #1 0x7f94f2047d9c in __interceptor_strspn /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:752
        #2 0x560e190f7f26 in parse_attr_line attr.c:375
        #3 0x560e190f9663 in handle_attr_line attr.c:660
        #4 0x560e190f9ddd in read_attr_from_index attr.c:769
        #5 0x560e190f9f14 in read_attr attr.c:797
        #6 0x560e190fa24e in bootstrap_attr_stack attr.c:867
        #7 0x560e190fa4a5 in prepare_attr_stack attr.c:902
        #8 0x560e190fb5dc in collect_some_attrs attr.c:1097
        #9 0x560e190fb93f in git_all_attrs attr.c:1128
        #10 0x560e18e6136e in check_attr builtin/check-attr.c:67
        #11 0x560e18e61c12 in cmd_check_attr builtin/check-attr.c:183
        #12 0x560e18e15993 in run_builtin git.c:466
        #13 0x560e18e16397 in handle_builtin git.c:721
        #14 0x560e18e16b2b in run_argv git.c:788
        #15 0x560e18e17991 in cmd_main git.c:926
        #16 0x560e190ae2bd in main common-main.c:57
        #17 0x7f94f1e3c28f  (/usr/lib/libc.so.6+0x2328f)
        #18 0x7f94f1e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #19 0x560e18e110e4 in _start ../sysdeps/x86_64/start.S:115

    AddressSanitizer can not provide additional info.
    SUMMARY: AddressSanitizer: SEGV (/usr/lib/libc.so.6+0x176082)
    ==8451==ABORTING

Fix this bug by converting the variable to a `size_t` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
It is possible to trigger an integer overflow when parsing attribute
names when there are more than 2^31 of them for a single pattern. This
can either lead to us dying due to trying to request too many bytes:

     blob=$(perl -e 'print "f" . " a=" x 2147483649' | git hash-object -w --stdin)
     git update-index --add --cacheinfo 100644,$blob,.gitattributes
     git attr-check --all file

    =================================================================
    ==1022==ERROR: AddressSanitizer: requested allocation size 0xfffffff800000032 (0xfffffff800001038 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
        #0 0x7fd3efabf411 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77
        #1 0x5563a0a1e3d3 in xcalloc wrapper.c:150
        #2 0x5563a058d005 in parse_attr_line attr.c:384
        #3 0x5563a058e661 in handle_attr_line attr.c:660
        #4 0x5563a058eddb in read_attr_from_index attr.c:769
        #5 0x5563a058ef12 in read_attr attr.c:797
        #6 0x5563a058f24c in bootstrap_attr_stack attr.c:867
        #7 0x5563a058f4a3 in prepare_attr_stack attr.c:902
        #8 0x5563a05905da in collect_some_attrs attr.c:1097
        #9 0x5563a059093d in git_all_attrs attr.c:1128
        #10 0x5563a02f636e in check_attr builtin/check-attr.c:67
        #11 0x5563a02f6c12 in cmd_check_attr builtin/check-attr.c:183
        #12 0x5563a02aa993 in run_builtin git.c:466
        #13 0x5563a02ab397 in handle_builtin git.c:721
        #14 0x5563a02abb2b in run_argv git.c:788
        #15 0x5563a02ac991 in cmd_main git.c:926
        #16 0x5563a05432bd in main common-main.c:57
        #17 0x7fd3ef82228f  (/usr/lib/libc.so.6+0x2328f)

    ==1022==HINT: if you don't care about these errors you may set allocator_may_return_null=1
    SUMMARY: AddressSanitizer: allocation-size-too-big /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 in __interceptor_calloc
    ==1022==ABORTING

Or, much worse, it can lead to an out-of-bounds write because we
underallocate and then memcpy(3P) into an array:

    perl -e '
        print "A " . "\rh="x2000000000;
        print "\rh="x2000000000;
        print "\rh="x294967294 . "\n"
    ' >.gitattributes
    git add .gitattributes
    git commit -am "evil attributes"

    $ git clone --quiet /path/to/repo
    =================================================================
    ==15062==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002550 at pc 0x5555559884d5 bp 0x7fffffffbc60 sp 0x7fffffffbc58
    WRITE of size 8 at 0x602000002550 thread T0
        #0 0x5555559884d4 in parse_attr_line attr.c:393
        #1 0x5555559884d4 in handle_attr_line attr.c:660
        #2 0x555555988902 in read_attr_from_index attr.c:784
        #3 0x555555988902 in read_attr_from_index attr.c:747
        #4 0x555555988a1d in read_attr attr.c:800
        #5 0x555555989b0c in bootstrap_attr_stack attr.c:882
        #6 0x555555989b0c in prepare_attr_stack attr.c:917
        #7 0x555555989b0c in collect_some_attrs attr.c:1112
        #8 0x55555598b141 in git_check_attr attr.c:1126
        #9 0x555555a13004 in convert_attrs convert.c:1311
        #10 0x555555a95e04 in checkout_entry_ca entry.c:553
        #11 0x555555d58bf6 in checkout_entry entry.h:42
        #12 0x555555d58bf6 in check_updates unpack-trees.c:480
        #13 0x555555d5eb55 in unpack_trees unpack-trees.c:2040
        #14 0x555555785ab7 in checkout builtin/clone.c:724
        #15 0x555555785ab7 in cmd_clone builtin/clone.c:1384
        #16 0x55555572443c in run_builtin git.c:466
        #17 0x55555572443c in handle_builtin git.c:721
        #18 0x555555727872 in run_argv git.c:788
        #19 0x555555727872 in cmd_main git.c:926
        #20 0x555555721fa0 in main common-main.c:57
        #21 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308
        #22 0x555555723f39 in _start (git+0x1cff39)

    0x602000002552 is located 0 bytes to the right of 2-byte region [0x602000002550,0x602000002552) allocated by thread T0 here:
        #0 0x7ffff768c037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
        #1 0x555555d7fff7 in xcalloc wrapper.c:150
        #2 0x55555598815f in parse_attr_line attr.c:384
        #3 0x55555598815f in handle_attr_line attr.c:660
        #4 0x555555988902 in read_attr_from_index attr.c:784
        #5 0x555555988902 in read_attr_from_index attr.c:747
        #6 0x555555988a1d in read_attr attr.c:800
        #7 0x555555989b0c in bootstrap_attr_stack attr.c:882
        #8 0x555555989b0c in prepare_attr_stack attr.c:917
        #9 0x555555989b0c in collect_some_attrs attr.c:1112
        #10 0x55555598b141 in git_check_attr attr.c:1126
        #11 0x555555a13004 in convert_attrs convert.c:1311
        #12 0x555555a95e04 in checkout_entry_ca entry.c:553
        #13 0x555555d58bf6 in checkout_entry entry.h:42
        #14 0x555555d58bf6 in check_updates unpack-trees.c:480
        #15 0x555555d5eb55 in unpack_trees unpack-trees.c:2040
        #16 0x555555785ab7 in checkout builtin/clone.c:724
        #17 0x555555785ab7 in cmd_clone builtin/clone.c:1384
        #18 0x55555572443c in run_builtin git.c:466
        #19 0x55555572443c in handle_builtin git.c:721
        #20 0x555555727872 in run_argv git.c:788
        #21 0x555555727872 in cmd_main git.c:926
        #22 0x555555721fa0 in main common-main.c:57
        #23 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308

    SUMMARY: AddressSanitizer: heap-buffer-overflow attr.c:393 in parse_attr_line
    Shadow bytes around the buggy address:
      0x0c047fff8450: fa fa 00 02 fa fa 00 07 fa fa fd fd fa fa 00 00
      0x0c047fff8460: fa fa 02 fa fa fa fd fd fa fa 00 06 fa fa 05 fa
      0x0c047fff8470: fa fa fd fd fa fa 00 02 fa fa 06 fa fa fa 05 fa
      0x0c047fff8480: fa fa 07 fa fa fa fd fd fa fa 00 01 fa fa 00 02
      0x0c047fff8490: fa fa 00 03 fa fa 00 fa fa fa 00 01 fa fa 00 03
    =>0x0c047fff84a0: fa fa 00 01 fa fa 00 02 fa fa[02]fa fa fa fa fa
      0x0c047fff84b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff84c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff84d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff84e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff84f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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
    ==15062==ABORTING

Fix this bug by using `size_t` instead to count the number of attributes
so that this value cannot reasonably overflow without running out of
memory before already.

Reported-by: Markus Vervier <markus.vervier@x41-dsec.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
When using a padding specifier in the pretty format passed to git-log(1)
we need to calculate the string length in several places. These string
lengths are stored in `int`s though, which means that these can easily
overflow when the input lengths exceeds 2GB. This can ultimately lead to
an out-of-bounds write when these are used in a call to memcpy(3P):

        ==8340==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f1ec62f97fe at pc 0x7f2127e5f427 bp 0x7ffd3bd63de0 sp 0x7ffd3bd63588
    WRITE of size 1 at 0x7f1ec62f97fe thread T0
        #0 0x7f2127e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
        #1 0x5628e96aa605 in format_and_pad_commit pretty.c:1762
        #2 0x5628e96aa7f4 in format_commit_item pretty.c:1801
        #3 0x5628e97cdb24 in strbuf_expand strbuf.c:429
        #4 0x5628e96ab060 in repo_format_commit_message pretty.c:1869
        #5 0x5628e96acd0f in pretty_print_commit pretty.c:2161
        #6 0x5628e95a44c8 in show_log log-tree.c:781
        #7 0x5628e95a76ba in log_tree_commit log-tree.c:1117
        #8 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508
        #9 0x5628e922c35b in cmd_log_walk builtin/log.c:549
        #10 0x5628e922f1a2 in cmd_log builtin/log.c:883
        #11 0x5628e9106993 in run_builtin git.c:466
        #12 0x5628e9107397 in handle_builtin git.c:721
        #13 0x5628e9107b07 in run_argv git.c:788
        #14 0x5628e91088a7 in cmd_main git.c:923
        #15 0x5628e939d682 in main common-main.c:57
        #16 0x7f2127c3c28f  (/usr/lib/libc.so.6+0x2328f)
        #17 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #18 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115

    0x7f1ec62f97fe is located 2 bytes to the left of 4831838265-byte region [0x7f1ec62f9800,0x7f1fe62f9839)
    allocated by thread T0 here:
        #0 0x7f2127ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
        #1 0x5628e98774d4 in xrealloc wrapper.c:136
        #2 0x5628e97cb01c in strbuf_grow strbuf.c:99
        #3 0x5628e97ccd42 in strbuf_addchars strbuf.c:327
        #4 0x5628e96aa55c in format_and_pad_commit pretty.c:1761
        #5 0x5628e96aa7f4 in format_commit_item pretty.c:1801
        #6 0x5628e97cdb24 in strbuf_expand strbuf.c:429
        #7 0x5628e96ab060 in repo_format_commit_message pretty.c:1869
        #8 0x5628e96acd0f in pretty_print_commit pretty.c:2161
        #9 0x5628e95a44c8 in show_log log-tree.c:781
        #10 0x5628e95a76ba in log_tree_commit log-tree.c:1117
        #11 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508
        #12 0x5628e922c35b in cmd_log_walk builtin/log.c:549
        #13 0x5628e922f1a2 in cmd_log builtin/log.c:883
        #14 0x5628e9106993 in run_builtin git.c:466
        #15 0x5628e9107397 in handle_builtin git.c:721
        #16 0x5628e9107b07 in run_argv git.c:788
        #17 0x5628e91088a7 in cmd_main git.c:923
        #18 0x5628e939d682 in main common-main.c:57
        #19 0x7f2127c3c28f  (/usr/lib/libc.so.6+0x2328f)
        #20 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #21 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115

    SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
    Shadow bytes around the buggy address:
      0x0fe458c572a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0fe458c572b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0fe458c572c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0fe458c572d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0fe458c572e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    =>0x0fe458c572f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
      0x0fe458c57300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
      0x0fe458c57310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
      0x0fe458c57320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
      0x0fe458c57330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
      0x0fe458c57340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    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
    ==8340==ABORTING

The pretty format can also be used in `git archive` operations via the
`export-subst` attribute. So this is what in our opinion makes this a
critical issue in the context of Git forges which allow to download an
archive of user supplied Git repositories.

Fix this vulnerability by using `size_t` instead of `int` to track the
string lengths. Add tests which detect this vulnerability when Git is
compiled with the address sanitizer.

Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Original-patch-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Modified-by: Taylor  Blau <me@ttalorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
With the `%>>(<N>)` pretty formatter, you can ask git-log(1) et al to
steal spaces. To do so we need to look ahead of the next token to see
whether there are spaces there. This loop takes into account ANSI
sequences that end with an `m`, and if it finds any it will skip them
until it finds the first space. While doing so it does not take into
account the buffer's limits though and easily does an out-of-bounds
read.

Add a test that hits this behaviour. While we don't have an easy way to
verify this, the test causes the following failure when run with
`SANITIZE=address`:

    ==37941==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000baf at pc 0x55ba6f88e0d0 bp 0x7ffc84c50d20 sp 0x7ffc84c50d10
    READ of size 1 at 0x603000000baf thread T0
        #0 0x55ba6f88e0cf in format_and_pad_commit pretty.c:1712
        #1 0x55ba6f88e7b4 in format_commit_item pretty.c:1801
        #2 0x55ba6f9b1ae4 in strbuf_expand strbuf.c:429
        #3 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869
        #4 0x55ba6f890ccf in pretty_print_commit pretty.c:2161
        #5 0x55ba6f7884c8 in show_log log-tree.c:781
        #6 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117
        #7 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508
        #8 0x55ba6f41035b in cmd_log_walk builtin/log.c:549
        #9 0x55ba6f4131a2 in cmd_log builtin/log.c:883
        #10 0x55ba6f2ea993 in run_builtin git.c:466
        #11 0x55ba6f2eb397 in handle_builtin git.c:721
        #12 0x55ba6f2ebb07 in run_argv git.c:788
        #13 0x55ba6f2ec8a7 in cmd_main git.c:923
        #14 0x55ba6f581682 in main common-main.c:57
        #15 0x7f2d08c3c28f  (/usr/lib/libc.so.6+0x2328f)
        #16 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #17 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115

    0x603000000baf is located 1 bytes to the left of 24-byte region [0x603000000bb0,0x603000000bc8)
    allocated by thread T0 here:
        #0 0x7f2d08ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
        #1 0x55ba6fa5b494 in xrealloc wrapper.c:136
        #2 0x55ba6f9aefdc in strbuf_grow strbuf.c:99
        #3 0x55ba6f9b0a06 in strbuf_add strbuf.c:298
        #4 0x55ba6f9b1a25 in strbuf_expand strbuf.c:418
        #5 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869
        #6 0x55ba6f890ccf in pretty_print_commit pretty.c:2161
        #7 0x55ba6f7884c8 in show_log log-tree.c:781
        #8 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117
        #9 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508
        #10 0x55ba6f41035b in cmd_log_walk builtin/log.c:549
        #11 0x55ba6f4131a2 in cmd_log builtin/log.c:883
        #12 0x55ba6f2ea993 in run_builtin git.c:466
        #13 0x55ba6f2eb397 in handle_builtin git.c:721
        #14 0x55ba6f2ebb07 in run_argv git.c:788
        #15 0x55ba6f2ec8a7 in cmd_main git.c:923
        #16 0x55ba6f581682 in main common-main.c:57
        #17 0x7f2d08c3c28f  (/usr/lib/libc.so.6+0x2328f)
        #18 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #19 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115

    SUMMARY: AddressSanitizer: heap-buffer-overflow pretty.c:1712 in format_and_pad_commit
    Shadow bytes around the buggy address:
      0x0c067fff8120: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
      0x0c067fff8130: fd fd fa fa fd fd fd fd fa fa fd fd fd fa fa fa
      0x0c067fff8140: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa
      0x0c067fff8150: fa fa fd fd fd fd fa fa 00 00 00 fa fa fa fd fd
      0x0c067fff8160: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
    =>0x0c067fff8170: fd fd fd fa fa[fa]00 00 00 fa fa fa 00 00 00 fa
      0x0c067fff8180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff8190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff81a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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

Luckily enough, this would only cause us to copy the out-of-bounds data
into the formatted commit in case we really had an ANSI sequence
preceding our buffer. So this bug likely has no security consequences.

Fix it regardless by not traversing past the buffer's start.

Reported-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
An out-of-bounds read can be triggered when parsing an incomplete
padding format string passed via `--pretty=format` or in Git archives
when files are marked with the `export-subst` gitattribute.

This bug exists since we have introduced support for truncating output
via the `trunc` keyword a7f01c6 (pretty: support truncating in %>, %<
and %><, 2013-04-19). Before this commit, we used to find the end of the
formatting string by using strchr(3P). This function returns a `NULL`
pointer in case the character in question wasn't found. The subsequent
check whether any character was found thus simply checked the returned
pointer. After the commit we switched to strcspn(3P) though, which only
returns the offset to the first found character or to the trailing NUL
byte. As the end pointer is now computed by adding the offset to the
start pointer it won't be `NULL` anymore, and as a consequence the check
doesn't do anything anymore.

The out-of-bounds data that is being read can in fact end up in the
formatted string. As a consequence, it is possible to leak memory
contents either by calling git-log(1) or via git-archive(1) when any of
the archived files is marked with the `export-subst` gitattribute.

    ==10888==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000398 at pc 0x7f0356047cb2 bp 0x7fff3ffb95d0 sp 0x7fff3ffb8d78
    READ of size 1 at 0x602000000398 thread T0
        #0 0x7f0356047cb1 in __interceptor_strchrnul /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725
        #1 0x563b7cec9a43 in strbuf_expand strbuf.c:417
        #2 0x563b7cda7060 in repo_format_commit_message pretty.c:1869
        #3 0x563b7cda8d0f in pretty_print_commit pretty.c:2161
        #4 0x563b7cca04c8 in show_log log-tree.c:781
        #5 0x563b7cca36ba in log_tree_commit log-tree.c:1117
        #6 0x563b7c927ed5 in cmd_log_walk_no_free builtin/log.c:508
        #7 0x563b7c92835b in cmd_log_walk builtin/log.c:549
        #8 0x563b7c92b1a2 in cmd_log builtin/log.c:883
        #9 0x563b7c802993 in run_builtin git.c:466
        #10 0x563b7c803397 in handle_builtin git.c:721
        #11 0x563b7c803b07 in run_argv git.c:788
        #12 0x563b7c8048a7 in cmd_main git.c:923
        #13 0x563b7ca99682 in main common-main.c:57
        #14 0x7f0355e3c28f  (/usr/lib/libc.so.6+0x2328f)
        #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115

    0x602000000398 is located 0 bytes to the right of 8-byte region [0x602000000390,0x602000000398)
    allocated by thread T0 here:
        #0 0x7f0356072faa in __interceptor_strdup /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:439
        #1 0x563b7cf7317c in xstrdup wrapper.c:39
        #2 0x563b7cd9a06a in save_user_format pretty.c:40
        #3 0x563b7cd9b3e5 in get_commit_format pretty.c:173
        #4 0x563b7ce54ea0 in handle_revision_opt revision.c:2456
        #5 0x563b7ce597c9 in setup_revisions revision.c:2850
        #6 0x563b7c9269e0 in cmd_log_init_finish builtin/log.c:269
        #7 0x563b7c927362 in cmd_log_init builtin/log.c:348
        #8 0x563b7c92b193 in cmd_log builtin/log.c:882
        #9 0x563b7c802993 in run_builtin git.c:466
        #10 0x563b7c803397 in handle_builtin git.c:721
        #11 0x563b7c803b07 in run_argv git.c:788
        #12 0x563b7c8048a7 in cmd_main git.c:923
        #13 0x563b7ca99682 in main common-main.c:57
        #14 0x7f0355e3c28f  (/usr/lib/libc.so.6+0x2328f)
        #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115

    SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 in __interceptor_strchrnul
    Shadow bytes around the buggy address:
      0x0c047fff8020: fa fa fd fd fa fa 00 06 fa fa 05 fa fa fa fd fd
      0x0c047fff8030: fa fa 00 02 fa fa 06 fa fa fa 05 fa fa fa fd fd
      0x0c047fff8040: fa fa 00 07 fa fa 03 fa fa fa fd fd fa fa 00 00
      0x0c047fff8050: fa fa 00 01 fa fa fd fd fa fa 00 00 fa fa 00 01
      0x0c047fff8060: fa fa 00 06 fa fa 00 06 fa fa 05 fa fa fa 05 fa
    =>0x0c047fff8070: fa fa 00[fa]fa fa fd fa fa fa fd fd fa fa fd fd
      0x0c047fff8080: fa fa fd fd fa fa 00 00 fa fa 00 fa fa fa fd fa
      0x0c047fff8090: fa fa fd fd fa fa 00 00 fa fa fa fa fa fa fa fa
      0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c047fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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
    ==10888==ABORTING

Fix this bug by checking whether `end` points at the trailing NUL byte.
Add a test which catches this out-of-bounds read and which demonstrates
that we used to write out-of-bounds data into the formatted message.

Reported-by: Markus Vervier <markus.vervier@x41-dsec.de>
Original-patch-by: Markus Vervier <markus.vervier@x41-dsec.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
derrickstolee pushed a commit that referenced this pull request Jan 17, 2023
The return type of both `utf8_strwidth()` and `utf8_strnwidth()` is
`int`, but we operate on string lengths which are typically of type
`size_t`. This means that when the string is longer than `INT_MAX`, we
will overflow and thus return a negative result.

This can lead to an out-of-bounds write with `--pretty=format:%<1)%B`
and a commit message that is 2^31+1 bytes long:

    =================================================================
    ==26009==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000001168 at pc 0x7f95c4e5f427 bp 0x7ffd8541c900 sp 0x7ffd8541c0a8
    WRITE of size 2147483649 at 0x603000001168 thread T0
        #0 0x7f95c4e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
        #1 0x5612bbb1068c in format_and_pad_commit pretty.c:1763
        #2 0x5612bbb1087a in format_commit_item pretty.c:1801
        #3 0x5612bbc33bab in strbuf_expand strbuf.c:429
        #4 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869
        #5 0x5612bbb12d96 in pretty_print_commit pretty.c:2161
        #6 0x5612bba0a4d5 in show_log log-tree.c:781
        #7 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117
        #8 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508
        #9 0x5612bb69235b in cmd_log_walk builtin/log.c:549
        #10 0x5612bb6951a2 in cmd_log builtin/log.c:883
        #11 0x5612bb56c993 in run_builtin git.c:466
        #12 0x5612bb56d397 in handle_builtin git.c:721
        #13 0x5612bb56db07 in run_argv git.c:788
        #14 0x5612bb56e8a7 in cmd_main git.c:923
        #15 0x5612bb803682 in main common-main.c:57
        #16 0x7f95c4c3c28f  (/usr/lib/libc.so.6+0x2328f)
        #17 0x7f95c4c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
        #18 0x5612bb5680e4 in _start ../sysdeps/x86_64/start.S:115

    0x603000001168 is located 0 bytes to the right of 24-byte region [0x603000001150,0x603000001168)
    allocated by thread T0 here:
        #0 0x7f95c4ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
        #1 0x5612bbcdd556 in xrealloc wrapper.c:136
        #2 0x5612bbc310a3 in strbuf_grow strbuf.c:99
        #3 0x5612bbc32acd in strbuf_add strbuf.c:298
        #4 0x5612bbc33aec in strbuf_expand strbuf.c:418
        #5 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869
        #6 0x5612bbb12d96 in pretty_print_commit pretty.c:2161
        #7 0x5612bba0a4d5 in show_log log-tree.c:781
        #8 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117
        #9 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508
        #10 0x5612bb69235b in cmd_log_walk builtin/log.c:549
        #11 0x5612bb6951a2 in cmd_log builtin/log.c:883
        #12 0x5612bb56c993 in run_builtin git.c:466
        #13 0x5612bb56d397 in handle_builtin git.c:721
        #14 0x5612bb56db07 in run_argv git.c:788
        #15 0x5612bb56e8a7 in cmd_main git.c:923
        #16 0x5612bb803682 in main common-main.c:57
        #17 0x7f95c4c3c28f  (/usr/lib/libc.so.6+0x2328f)

    SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
    Shadow bytes around the buggy address:
      0x0c067fff81d0: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa
      0x0c067fff81e0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
      0x0c067fff81f0: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
      0x0c067fff8200: fd fd fd fa fa fa fd fd fd fd fa fa 00 00 00 fa
      0x0c067fff8210: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
    =>0x0c067fff8220: fd fa fa fa fd fd fd fa fa fa 00 00 00[fa]fa fa
      0x0c067fff8230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff8240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c067fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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
    ==26009==ABORTING

Now the proper fix for this would be to convert both functions to return
an `size_t` instead of an `int`. But given that this commit may be part
of a security release, let's instead do the minimal viable fix and die
in case we see an overflow.

Add a test that would have previously caused us to crash.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Aug 25, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7
(push: introduce '--branches' option, 2023-05-06), it was not leak-free.
In fact, the test did not even run correctly until 022fbb6 (t5583:
fix shebang line, 2023-05-12), but after applying that patch, we see a
failure at t5583.8:

    ==2529087==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 384 byte(s) in 1 object(s) allocated from:
        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
        #1 0x55e07606cbf9 in xrealloc wrapper.c:140
        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42
        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917
        #4 0x55e075fe9cce in add_missing_tags remote.c:1518
        #5 0x55e075fea1e4 in match_push_refs remote.c:1665
        #6 0x55e076050a8e in transport_push transport.c:1378
        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401
        #8 0x55e075e2edb0 in do_push builtin/push.c:458
        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702
        #10 0x55e075d8aaf0 in run_builtin git.c:452
        #11 0x55e075d8af08 in handle_builtin git.c:706
        #12 0x55e075d8b12c in run_argv git.c:770
        #13 0x55e075d8b6a0 in cmd_main git.c:905
        #14 0x55e075e81f07 in main common-main.c:60
        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360
        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)

    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).

This leak was addressed independently via 68b5117 (commit-reach: fix
memory leak in get_reachable_subset(), 2023-06-03), which makes t5583
leak-free.

But t5583 was not in the tree when 68b5117 was written, and the two
only met after the latter was merged back in via 693bde4 (Merge
branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).

At that point, t5583 was leak-free. Let's mark it as such accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-for-windows-ci pushed a commit that referenced this pull request Aug 29, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7
(push: introduce '--branches' option, 2023-05-06), it was not leak-free.
In fact, the test did not even run correctly until 022fbb6 (t5583:
fix shebang line, 2023-05-12), but after applying that patch, we see a
failure at t5583.8:

    ==2529087==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 384 byte(s) in 1 object(s) allocated from:
        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
        #1 0x55e07606cbf9 in xrealloc wrapper.c:140
        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42
        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917
        #4 0x55e075fe9cce in add_missing_tags remote.c:1518
        #5 0x55e075fea1e4 in match_push_refs remote.c:1665
        #6 0x55e076050a8e in transport_push transport.c:1378
        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401
        #8 0x55e075e2edb0 in do_push builtin/push.c:458
        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702
        #10 0x55e075d8aaf0 in run_builtin git.c:452
        #11 0x55e075d8af08 in handle_builtin git.c:706
        #12 0x55e075d8b12c in run_argv git.c:770
        #13 0x55e075d8b6a0 in cmd_main git.c:905
        #14 0x55e075e81f07 in main common-main.c:60
        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360
        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)

    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).

This leak was addressed independently via 68b5117 (commit-reach: fix
memory leak in get_reachable_subset(), 2023-06-03), which makes t5583
leak-free.

But t5583 was not in the tree when 68b5117 was written, and the two
only met after the latter was merged back in via 693bde4 (Merge
branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).

At that point, t5583 was leak-free. Let's mark it as such accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho pushed a commit that referenced this pull request Jan 9, 2024
The t5309 script triggers a racy false positive with SANITIZE=leak on a
multi-core system. Running with "--stress --run=6" usually fails within
10 seconds or so for me, complaining with something like:

    + git index-pack --fix-thin --stdin
    fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)

    =================================================================
    ==3904583==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 32 byte(s) in 1 object(s) allocated from:
        #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
        #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
        #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
        #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598
        #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51
        #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440
        #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444
        #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

    SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).
    Aborted

What happens is this:

  0. We construct a bogus pack with a duplicate object in it and trigger
     index-pack.

  1. We spawn a bunch of worker threads to resolve deltas (on my system
     it is 16 threads).

  2. One of the threads sees the duplicate object and bails by calling
     exit(), taking down all of the threads. This is expected and is the
     point of the test.

  3. At the time exit() is called, we may still be spawning threads from
     the main process via pthread_create(). LSan hooks thread creation
     to update its book-keeping; it has to know where each thread's
     stack is (so it can find entry points for reachable memory). So it
     calls pthread_getattr_np() to get information about the new thread.
     That may allocate memory that must be freed with a matching call to
     pthread_attr_destroy(). Probably LSan does that immediately, but
     if you're unlucky enough, the exit() will happen while it's between
     those two calls, and the allocated pthread_attr_t appears as a
     leak.

This isn't a real leak. It's not even in our code, but rather in the
LSan instrumentation code. So we could just ignore it. But the false
positive can cause people to waste time tracking it down.

It's possibly something that LSan could protect against (e.g., cover the
getattr/destroy pair with a mutex, and then in the final post-exit()
check for leaks try to take the same mutex). But I don't know enough
about LSan to say if that's a reasonable approach or not (or if my
analysis is even completely correct).

In the meantime, it's pretty easy to avoid the race by making creation
of the worker threads "atomic". That is, we'll spawn all of them before
letting any of them start to work. That's easy to do because we already
have a work_lock() mutex for handing out that work. If the main process
takes it, then all of the threads will immediately block until we've
finished spawning and released it.

This shouldn't make any practical difference for non-LSan runs. The
thread spawning is quick, and could happen before any worker thread gets
scheduled anyway.

Probably other spots that use threads are subject to the same issues.
But since we have to manually insert locking (and since this really is
kind of a hack), let's not bother with them unless somebody experiences
a similar racy false-positive in practice.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-for-windows-ci pushed a commit that referenced this pull request Jun 11, 2024
When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()
is responsible for generating an array of bitmapped_pack structs from
which to perform reuse.

In the multi-pack case, we loop over the MIDXs packs and copy the result
of calling `nth_bitmapped_pack()` to construct the list of reusable
paths.

But we may also want to do pack-reuse over a single pack, either because
we only had one pack to perform reuse over (in the case of single-pack
bitmaps), or because we explicitly asked to do single pack reuse even
with a MIDX[^1].

When this is the case, the array we generate of reusable packs contains
only a single element, which is either (a) the pack attached to the
single-pack bitmap, or (b) the MIDX's preferred pack.

In 795006f (pack-bitmap: gracefully handle missing BTMP chunks,
2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()
function and stopped assigning the pack_int_id field when reusing only
the MIDX's preferred pack. This results in an uninitialized read down in
try_partial_reuse() like so:

    ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8
    #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8
    #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3
    #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3
    #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27
    #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3
    #6 0x55c5ccc8fac8 in run_builtin git.c:474:11

which happens when try_partial_reuse() tries to call
midx_pair_to_pack_pos() when it tries to reject cross-pack deltas.

Avoid the uninitialized read by ensuring that the pack_int_id field is
set in the single-pack reuse case by setting it to either the MIDX
preferred pack's pack_int_id, or '-1', in the case of single-pack
bitmaps.  In the latter case, we never read the pack_int_id field, so
the choice of '-1' is intentional as a "garbage in, garbage out"
measure.

Guard against further regressions in this area by adding a test which
ensures that we do not throw out deltas from the preferred pack as
"cross-pack" due to an uninitialized pack_int_id.

[^1]: This can happen for a couple of reasons, either because the
  repository is configured with 'pack.allowPackReuse=(true|single)', or
  because the MIDX was generated prior to the introduction of the BTMP
  chunk, which contains information necessary to perform multi-pack
  reuse.

Reported-by: Kyle Lippincott <spectral@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-for-windows-ci pushed a commit that referenced this pull request Jun 17, 2024
Memory sanitizer (msan) is detecting a use of an uninitialized variable
(`size`) in `read_attr_from_index`:

    ==2268==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11
    #1 0x5651f3415530 in read_attr git/attr.c
    #2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6
    #3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2
    #4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2
    #5 0x5651f3413144 in git_check_attr git/attr.c:1345:2
    #6 0x5651f34728da in convert_attrs git/convert.c:1320:2
    #7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2
    #8 0x5651f357a35e in index_fd git/object-file.c:2630:34
    #9 0x5651f357aa15 in index_path git/object-file.c:2657:7
    #10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7
    #11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9
    #12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7
    #13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18
    #14 0x5651f321d327 in run_builtin git/git.c:474:11
    #15 0x5651f321bc9e in handle_builtin git/git.c:729:3
    #16 0x5651f321a792 in run_argv git/git.c:793:4
    #17 0x5651f321a792 in cmd_main git/git.c:928:19
    #18 0x5651f33dde1f in main git/common-main.c:62:11

The issue exists because `size` is an output parameter from
`read_blob_data_from_index`, but it's only modified if
`read_blob_data_from_index` returns non-NULL. The read of `size` when
calling `read_attr_from_buf` unconditionally may read from an
uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
before reading from `size`, but by then it's already too late: the
uninitialized read will have happened already. Furthermore, there's no
guarantee that the compiler won't reorder things so that it checks
`size` before checking `!buf`.

Make the call to `read_attr_from_buf` conditional on `buf` being
non-NULL, ensuring that `size` is not read if it's never set.

Signed-off-by: Kyle Lippincott <spectral@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants