Fast track bug fix to merge-recursive #3

benpeart · 2018-07-23T18:09:21Z

Changes in 2.18.0 induced a bug in merge_content() when merging files that are not present in the local working directory due to sparse-checkout. Fast track the bug fix into our fork as GVFS requires correct behavior with sparse-checkout.

Recent changes in merge_content() induced a bug when merging files that are not present in the local working directory due to sparse-checkout. Add a test case to demonstrate the bug so that we can ensure the fix resolves it and to prevent future regressions. Signed-off-by: Ben Peart <benpeart@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>

merge-recursive takes any files marked as unmerged by unpack_trees, tries to figure out whether they can be resolved (e.g. using renames or a file-level merge), and then if they can be it will delete the old cache entries and writes new ones. This means that any ce_flags for those cache entries are essentially cleared when merging. Unfortunately, if a file was marked as skip_worktree and it needs a file-level merge but the merge results in the same version of the file that was found in HEAD, we skip updating the worktree (because the file was unchanged) but clear the skip_worktree bit (because of the delete-cache-entry-and-write-new-one). This makes git treat the file as having a local change in the working copy, namely a delete, when it should appear as unchanged despite not being present. Avoid this problem by copying the skip_worktree flag in this case. Signed-off-by: Elijah Newren <newren@gmail.com>

dscho · 2018-07-26T16:31:58Z

Thanks for taking the time to contribute to Git!

Maybe we want to replace the PR description with something that really describes this PR?

…ev() In 'builtin/name-rev.c' in the name_rev() function there is a loop iterating over all parents of the given commit, and the loop body looks like this: if (parent_number > 1) { if (generation > 0) // branch #1 new_name = ... else // branch #2 new_name = ... name_rev(parent, new_name, ...); } else { // branch #3 name_rev(...); } These conditions are not covered properly in the test suite. As far as purely test coverage goes, they are all executed several times over in 't6120-describe.sh'. However, they don't directly influence the command's output, because the repository used in that test script contains several branches and tags pointing somewhere into the middle of the commit DAG, and thus result in a better name for the to-be-named commit. This can hide bugs: e.g. by replacing the 'new_name' parameter of the first recursive name_rev() call with 'tip_name' (effectively making both branch #1 and #2 a noop) 'git name-rev --all' shows thousands of bogus names in the Git repository, but the whole test suite still passes successfully. In an early version of a later patch in this series I managed to mess up all three branches (at once!), but the test suite still passed. So add a new test case that operates on the following history: A--------------master \ / \----------M2 \ / \---M1-C \ / B and names the commit 'B' to make sure that all three branches are crucial to determine 'B's name: - There is only a single ref, so all names are based on 'master', without any undesired interference from other refs. - Each time name_rev() follows the second parent of a merge commit, it appends "^2" to the name. Following 'master's second parent right at the start ensures that all commits on the ancestry path from 'master' to 'B' have a different base name from the original 'tip_name' of the very first name_rev() invocation. Currently, while name_rev() is recursive, it doesn't matter, but it will be necessary to properly cover all three branches after the recursion is eliminated later in this series. - Following 'M2's second parent makes sure that branch #2 (i.e. when 'generation = 0') affects 'B's name. - Following the only parent of the non-merge commit 'C' ensures that branch #3 affects 'B's name, and that it increments 'generation'. - Coming from 'C' 'generation' is 1, thus following 'M1's second parent makes sure that branch #1 affects 'B's name. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Recent versions of the gcc and clang Address Sanitizer produce test failures related to regexec(). This triggers with gcc-10 and clang-8 (but not gcc-9 nor clang-7). Running: make CC=gcc-10 SANITIZE=address test results in failures in t4018, t3206, and t4062. The cause seems to be that when built with ASan, we use a different version of regexec() than normal. And this version doesn't understand the REG_STARTEND flag. Here's my evidence supporting that. The failure in t4062 is an ASan warning: expecting success of 4062.2 '-G matches': git diff --name-only -G "^(0{64}){64}$" HEAD^ >out && test 4096-zeroes.txt = "$(cat out)" ================================================================= ==672994==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fa76f672000 at pc 0x7fa7726f75b6 bp 0x7ffe41bdda70 sp 0x7ffe41bdd220 READ of size 4097 at 0x7fa76f672000 thread T0 #0 0x7fa7726f75b5 (/lib/x86_64-linux-gnu/libasan.so.6+0x4f5b5) #1 0x562ae0c9c40e in regexec_buf /home/peff/compile/git/git-compat-util.h:1117 #2 0x562ae0c9c40e in diff_grep /home/peff/compile/git/diffcore-pickaxe.c:52 #3 0x562ae0c9cc28 in pickaxe_match /home/peff/compile/git/diffcore-pickaxe.c:166 [...] In this case we're looking in a buffer which was mmap'd via reuse_worktree_file(), and whose size is 4096 bytes. But libasan's regex tries to look at byte 4097 anyway! If we tweak Git like this: diff --git a/diff.c b/diff.c index 8e2914c031..cfae60c120 100644 --- a/diff.c +++ b/diff.c @@ -3880,7 +3880,7 @@ static int reuse_worktree_file(struct index_state *istate, */ if (ce_uptodate(ce) || (!lstat(name, &st) && !ie_match_stat(istate, ce, &st, 0))) - return 1; + return 0; return 0; } to use a regular buffer (with a trailing NUL) instead of an mmap, then the complaint goes away. The other failures are actually diff output with an incorrect funcname header. If I instrument xdiff to show the funcname matching like so: diff --git a/xdiff-interface.c b/xdiff-interface.c index 8509f9ea22..f6c3dc1986 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -197,6 +197,7 @@ struct ff_regs { struct ff_reg { regex_t re; int negate; + char *printable; } *array; }; @@ -218,7 +219,12 @@ static long ff_regexp(const char *line, long len, for (i = 0; i < regs->nr; i++) { struct ff_reg *reg = regs->array + i; - if (!regexec_buf(&reg->re, line, len, 2, pmatch, 0)) { + int ret = regexec_buf(&reg->re, line, len, 2, pmatch, 0); + warning("regexec %s:\n regex: %s\n buf: %.*s", + ret == 0 ? "matched" : "did not match", + reg->printable, + (int)len, line); + if (!ret) { if (reg->negate) return -1; break; @@ -264,6 +270,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *value, int cflags) expression = value; if (regcomp(&reg->re, expression, cflags)) die("Invalid regexp to look for hunk header: %s", expression); + reg->printable = xstrdup(expression); free(buffer); value = ep + 1; } then when compiling with ASan and gcc-10, running the diff from t4018.66 produces this: $ git diff -U1 cpp-skip-access-specifiers warning: regexec did not match: regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*]) buf: private: warning: regexec matched: regex: ^((::[[:space:]]*)?[A-Za-z_].*)$ buf: private: diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers index 4d4a9db..ebd6f42 100644 --- a/cpp-skip-access-specifiers +++ b/cpp-skip-access-specifiers @@ -6,3 +6,3 @@ private: void DoSomething(); int ChangeMe; }; void DoSomething(); - int ChangeMe; + int IWasChanged; }; That first regex should match (and is negated, so it should be telling us _not_ to match "private:"). But it wouldn't if regexec() is looking at the whole buffer, and not just the length-limited line we've fed to regexec_buf(). So this is consistent again with REG_STARTEND being ignored. The correct output (compiling without ASan, or gcc-9 with Asan) looks like this: warning: regexec matched: regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*]) buf: private: [...more lines that we end up not using...] warning: regexec matched: regex: ^((::[[:space:]]*)?[A-Za-z_].*)$ buf: class RIGHT : public Baseclass diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers index 4d4a9db..ebd6f42 100644 --- a/cpp-skip-access-specifiers +++ b/cpp-skip-access-specifiers @@ -6,3 +6,3 @@ class RIGHT : public Baseclass void DoSomething(); - int ChangeMe; + int IWasChanged; }; So it really does seem like libasan's regex engine is ignoring REG_STARTEND. We should be able to work around it by compiling with NO_REGEX, which would use our local regexec(). But to make matters even more interesting, this isn't enough by itself. Because ASan has support from the compiler, it doesn't seem to intercept our call to regexec() at the dynamic library level. It actually recognizes when we are compiling a call to regexec() and replaces it with ASan-specific code at that point. And unlike most of our other compat code, where we might have git_mmap() or similar, the actual symbol name in the compiled compat/regex code is regexec(). So just compiling with NO_REGEX isn't enough; we still end up in libasan! We can work around that by having the preprocessor replace regexec with git_regexec (both in the callers and in the actual implementation), and we truly end up with a call to our custom regex code, even when compiling with ASan. That's probably a good thing to do anyway, as it means anybody looking at the symbols later (e.g., in a debugger) would have a better indication of which function is which. So we'll do the same for the other common regex functions (even though just regexec() is enough to fix this ASan problem). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

The OFFSETOF_VAR(var, member) macro is implemented in terms of offsetof(typeof(*var), member) with compilers that know typeof(), but its fallback implemenation compares &(var->member) and (var) and count the distance in bytes, i.e. ((uintptr_t)&(var)->member - (uintptr_t)(var)) MSVC's runtime check, when fed an uninitialized 'var', flags this as a use of an uninitialized variable (and that is legit---uninitialized contents of 'var' is subtracted) in a debug build. After auditing all 6 uses of OFFSETOF_VAR(), 1 of them does feed a potentially uninitialized 'var' to the macro in the beginning of the for() loop: #define hashmap_for_each_entry(map, iter, var, member) \ for (var = hashmap_iter_first_entry_offset(map, iter, \ OFFSETOF_VAR(var, member)); \ var; \ var = hashmap_iter_next_entry_offset(iter, \ OFFSETOF_VAR(var, member))) We can work around this by making sure that var has _some_ value when OFFSETOF_VAR() is called. Strictly speaking, it invites undefined behaviour to use NULL here if we end up with pointer comparison, but MSVC runtime seems to be happy with it, and most other systems have typeof() and don't even need pointer comparison fallback code. Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Using the CMake support we added some time ago for real with Visual Studio build revealed there were lot of usability improvements possible, which have been carried out. * js/cmake-vs: hashmap_for_each_entry(): workaround MSVC's runtime check failure #3 cmake (Windows): recommend using Visual Studio's built-in CMake support cmake (Windows): initialize vcpkg/build dependencies automatically cmake (Windows): complain when encountering an unknown compiler cmake (Windows): let the `.dll` files be found when running the tests cmake: quote the path accurately when editing `test-lib.sh` cmake: fall back to using `vcpkg`'s `msgfmt.exe` on Windows cmake: ensure that the `vcpkg` packages are found on Windows cmake: do find Git for Windows' shell interpreter cmake: ignore files generated by CMake as run in Visual Studio

Test 5572.63 ("branch has no merge base with remote-tracking counterpart") was introduced in 4d36f88 (submodule: do not pass null OID to setup_revisions, 2018-05-24), as a regression test for the bug this commit was fixing (preventing a 'fatal: bad object' error when the current branch and the remote-tracking branch we are pulling have no merge-base). However, the commit message for 4d36f88 does not describe in which real-life situation this bug was encountered. The brief discussion on the mailing list [1] does not either. The regression test is not really representative of a real-life scenario: both the local repository and its upstream have only a single commit, and the "no merge-base" scenario is simulated by recreating this root commit in the local repository using 'git commit-tree' before calling 'git pull --rebase --recurse-submodules'. The rebase succeeds and results in the local branch being reset to the same root commit as the upstream branch. The fix in 4d36f88 modifies 'submodule.c::submodule_touches_in_range' so that if 'excl_oid' is null, which is the case when the 'git merge-base --fork-point' invocation in 'builtin/pull.c::get_rebase_fork_point' errors (no fork-point), then instead of 'incl_oid --not excl_oid' being passed to setup_revisions, only 'incl_oid' is passed, and 'submodule_touches_in_range' examines 'incl_oid' and all its ancestors to verify that they do not touch the submodule. In test 5572.63, the recreated lone root commit in the local repository is thus the only commit being examined by 'submodule_touches_in_range', and this commit *adds* the submodule. However, 'submodule_touches_in_range' *succeeds* because 'combine-diff.c::diff_tree_combined' (see the backtrace below) returns early since this commit is the root commit and has no parents. #0 diff_tree_combined at combine-diff.c:1494 #1 0x0000000100150cbe in diff_tree_combined_merge at combine-diff.c:1649 #2 0x00000001002c7147 in collect_changed_submodules at submodule.c:869 #3 0x00000001002c7d6f in submodule_touches_in_range at submodule.c:1268 #4 0x00000001000ad58b in cmd_pull at builtin/pull.c:1040 In light of all this, add a note in t5572 documenting this peculiar test. [1] https://lore.kernel.org/git/20180524204729.19896-1-jonathantanmy@google.com/t/#u Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

The previous change reduced time spent in strlen() while comparing consecutive paths in verify_cache(), but we can do better. The conditional checks the existence of a directory separator at the correct location, but only after doing a string comparison. Swap the order to be logically equivalent but perform fewer string comparisons. To test the effect on performance, I used a repository with over three million paths in the index. I then ran the following command on repeat: git -c index.threads=1 commit --amend --allow-empty --no-edit Here are the measurements over 10 runs after a 5-run warmup: Benchmark #1: v2.30.0 Time (mean ± σ): 854.5 ms ± 18.2 ms Range (min … max): 825.0 ms … 892.8 ms Benchmark microsoft#2: Previous change Time (mean ± σ): 833.2 ms ± 10.3 ms Range (min … max): 815.8 ms … 849.7 ms Benchmark microsoft#3: This change Time (mean ± σ): 815.5 ms ± 18.1 ms Range (min … max): 795.4 ms … 849.5 ms This change is 2% faster than the previous change and 5% faster than v2.30.0. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

This is modelled on the version of handle_directory_level_conflicts() from merge-recursive.c, but is massively simplified due to the following factors: * strmap API provides simplifications over using direct hashmap * we have a dirs_removed field in struct rename_info that we have an easy way to populate from collect_merge_info(); this was already used in compute_rename_counts() and thus we do not need to check for condition microsoft#2. * The removal of condition microsoft#2 by handling it earlier in the code also obviates the need to check for condition microsoft#3 -- if both sides renamed a directory, meaning that the directory no longer exists on either side, then neither side could have added any new files to that directory, and thus there are no files whose locations we need to move due to such a directory rename. In fact, the same logic that makes condition microsoft#3 irrelevant means condition #1 is also irrelevant so we could drop this function. However, it is cheap to check if both sides rename the same directory, and doing so can save future computation. So, simply remove any directories that both sides renamed from the list of directory renames. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Add some timing instrumentation for both merge-ort and diffcore-rename; I used these to measure and optimize performance in both, and several future patch series will build on these to reduce the timings of some select testcases. === Setup === The primary testcase I used involved rebasing a random topic in the linux kernel (consisting of 35 patches) against an older version. I added two variants, one where I rename a toplevel directory, and another where I only rebase one patch instead of the whole topic. The setup is as follows: $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git $ git branch hwmon-updates fd8bdb23b91876ac1e624337bb88dc1dcc21d67e $ git branch hwmon-just-one fd8bdb23b91876ac1e624337bb88dc1dcc21d67e~34 $ git branch base 4703d9119972bf586d2cca76ec6438f819ffa30e $ git switch -c 5.4-renames v5.4 $ git mv drivers pilots # Introduce over 26,000 renames $ git commit -m "Rename drivers/ to pilots/" $ git config merge.renameLimit 30000 $ git config merge.directoryRenames true === Testcases === Now with REBASE standing for either "git rebase [--merge]" (using merge-recursive) or "test-tool fast-rebase" (using merge-ort), the testcases are: Testcase #1: no-renames $ git checkout v5.4^0 $ REBASE --onto HEAD base hwmon-updates Note: technically the name is misleading; there are some renames, but very few. Rename detection only takes about half the overall time. Testcase microsoft#2: mega-renames $ git checkout 5.4-renames^0 $ REBASE --onto HEAD base hwmon-updates Testcase microsoft#3: just-one-mega $ git checkout 5.4-renames^0 $ REBASE --onto HEAD base hwmon-just-one === Timing results === Overall timings, using hyperfine (1 warmup run, 3 runs for mega-renames, 10 runs for the other two cases): merge-recursive merge-ort no-renames: 18.912 s ± 0.174 s 14.263 s ± 0.053 s mega-renames: 5964.031 s ± 10.459 s 5504.231 s ± 5.150 s just-one-mega: 149.583 s ± 0.751 s 158.534 s ± 0.498 s A single re-run of each with some breakdowns: --- no-renames --- merge-recursive merge-ort overall runtime: 19.302 s 14.257 s inexact rename detection: 7.603 s 7.906 s everything else: 11.699 s 6.351 s --- mega-renames --- merge-recursive merge-ort overall runtime: 5950.195 s 5499.672 s inexact rename detection: 5746.309 s 5487.120 s everything else: 203.886 s 17.552 s --- just-one-mega --- merge-recursive merge-ort overall runtime: 151.001 s 158.582 s inexact rename detection: 143.448 s 157.835 s everything else: 7.553 s 0.747 s === Timing observations === 0) Maximum speedup The "everything else" row represents the maximum speedup we could achieve if we were to somehow infinitely parallelize inexact rename detection, but leave everything else alone. The fact that this is so much smaller than the real runtime (even in the case with virtually no renames) makes it clear just how overwhelmingly large the time spent on rename detection can be. 1) no-renames 1a) merge-ort is faster than merge-recursive, which is nice. However, this still should not be considered good enough. Although the "merge" backend to rebase (merge-recursive) is sometimes faster than the "apply" backend, this is one of those cases where it is not. In fact, even merge-ort is slower. The "apply" backend can complete this testcase in 6.940 s ± 0.485 s which is about 2x faster than merge-ort and 3x faster than merge-recursive. One goal of the merge-ort performance work will be to make it faster than git-am on this (and similar) testcases. 2) mega-renames 2a) Obviously rename detection is a huge cost; it's where most the time is spent. We need to cut that down. If we could somehow infinitely parallelize it and drive its time to 0, the merge-recursive time would drop to about 204s, and the merge-ort time would drop to about 17s. I think this particular stat shows I've subtly baked a couple performance improvements into merge-ort and into fast-rebase already. 3) just-one-mega 3a) not much to say here, it just gives some flavor for how rebasing only one patch compares to rebasing 35. === Goals === This patch is obviously just the beginning. Here are some of my goals that this measurement will help us achieve: * Drive the cost of rename detection down considerably for merges * After the above has been achieved, see if there are other slowness factors (which would have previously been overshadowed by rename detection costs) which we can then focus on and also optimize. * Ensure our rebase testcase that requires little rename detection is noticeably faster with merge-ort than with apply-based rebase. Signed-off-by: Elijah Newren <newren@gmail.com> Acked-by: Taylor Blau <ttaylorr@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

…sponse query_result can be be an empty strbuf (STRBUF_INIT) - in that case trying to read 3 bytes triggers a buffer overflow read (as query_result.buf = '\0'). Therefore we need to check query_result's length before trying to read 3 bytes. This overflow was introduced in: 940b94f (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03) It was found when running the test-suite against ASAN, and can be most easily reproduced with the following command: make GIT_TEST_OPTS="-v" DEFAULT_TEST_TARGET="t7519-status-fsmonitor.sh" \ SANITIZE=address DEVELOPER=1 test ==2235==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000019e6e5e at pc 0x00000043745c bp 0x7fffd382c520 sp 0x7fffd382bcc8 READ of size 3 at 0x0000019e6e5e thread T0 #0 0x43745b in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7 #1 0x43786d in bcmp /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:887:10 #2 0x80b146 in fsmonitor_is_trivial_response /home/ahunt/oss-fuzz/git/fsmonitor.c:192:10 #3 0x80b146 in query_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:175:7 #4 0x80a749 in refresh_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:267:21 #5 0x80bad1 in tweak_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:429:4 #6 0x90f040 in read_index_from /home/ahunt/oss-fuzz/git/read-cache.c:2321:3 #7 0x8e5d08 in repo_read_index_preload /home/ahunt/oss-fuzz/git/preload-index.c:164:15 #8 0x52dd45 in prepare_index /home/ahunt/oss-fuzz/git/builtin/commit.c:363:6 #9 0x52a188 in cmd_commit /home/ahunt/oss-fuzz/git/builtin/commit.c:1588:15 #10 0x4ce77e in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 #11 0x4ccb18 in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 #12 0x4cb01c in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 #13 0x4cb01c in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 #14 0x6aca8d in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 #15 0x7fb027bf5349 in __libc_start_main (/lib64/libc.so.6+0x24349) #16 0x4206b9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120 0x0000019e6e5e is located 2 bytes to the left of global variable 'strbuf_slopbuf' defined in 'strbuf.c:51:6' (0x19e6e60) of size 1 'strbuf_slopbuf' is ascii string '' 0x0000019e6e5e is located 126 bytes to the right of global variable 'signals' defined in 'sigchain.c:11:31' (0x19e6be0) of size 512 SUMMARY: AddressSanitizer: global-buffer-overflow /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7 in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) Shadow bytes around the buggy address: 0x000080334d70: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x000080334d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080334d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080334da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080334db0: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 =>0x000080334dc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9[f9]01 f9 f9 f9 0x000080334dd0: f9 f9 f9 f9 03 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 0x000080334de0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 0x000080334df0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x000080334e00: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 01 f9 f9 f9 0x000080334e10: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 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 Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Acked-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Commit 878f988 (t/test-lib: teach --chain-lint to detect broken &&-chains in subshells, 2018-07-11) introduced additional chain-lint tests which add an extra "sed" pipeline to each test we run. This has a measurable impact on runtime. Here are timings with and without a new environment variable (added by this patch) that lets you disable just the additional sed-based chain-lint tests: Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 make test Time (mean ± σ): 64.202 s ± 1.030 s [User: 622.469 s, System: 301.402 s] Range (min … max): 61.571 s … 65.662 s 10 runs Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 make test Time (mean ± σ): 57.591 s ± 0.333 s [User: 529.368 s, System: 270.618 s] Range (min … max): 57.143 s … 58.309 s 10 runs Summary 'GIT_TEST_CHAIN_LINT_HARDER=0 make test' ran 1.11 ± 0.02 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 make test' Of course those extra lint checks are doing something useful, so paying a few extra seconds (at least on Linux) isn't so bad (though note the CPU time; we're bounded in our parallel run here by the slowest test, so it really is ~120s of CPU improvement). But we can observe that there are some test scripts where they produce a much stronger effect, and provide less value. In t0027 and t3070 we run a very large number of small tests, all driven by a series of functions/loops which are filling in the test bodies. There we get much less bang for our buck in terms of bug-finding versus CPU cost. This patch introduces a mechanism for controlling when those extra lint checks are run, at two levels: - a user can ask to disable or to force-enable the checks by setting GIT_TEST_CHAIN_LINT_HARDER - if the user hasn't specified a preference, individual scripts can disable the checks by setting GIT_TEST_CHAIN_LINT_HARDER_DEFAULT; scripts which don't set that get the current behavior of enabling them. In addition, this patch flips the default for t0027 and t3070's mass-generated sections to disable the extra checks. Here are the timing results for t0027: Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 ./t0027-auto-crlf.sh Time (mean ± σ): 17.078 s ± 0.848 s [User: 14.878 s, System: 7.075 s] Range (min … max): 15.952 s … 18.421 s 10 runs Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 ./t0027-auto-crlf.sh Time (mean ± σ): 9.063 s ± 0.759 s [User: 7.890 s, System: 3.362 s] Range (min … max): 7.747 s … 10.619 s 10 runs Benchmark #3: ./t0027-auto-crlf.sh Time (mean ± σ): 9.186 s ± 0.881 s [User: 7.957 s, System: 3.427 s] Range (min … max): 7.796 s … 10.498 s 10 runs Summary 'GIT_TEST_CHAIN_LINT_HARDER=0 ./t0027-auto-crlf.sh' ran 1.01 ± 0.13 times faster than './t0027-auto-crlf.sh' 1.88 ± 0.18 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 ./t0027-auto-crlf.sh' We can see that disabling the checks for the whole script buys us an almost 2x speedup. But the new default behavior, disabling them only for the mass-generated part, gets us most of that speedup (but still leaves the checks on for further manual tests people might write). As a side note, I'd caution about comparing runtimes and CPU seconds between this timing and the earlier "make test" one. In "make test", we're running a lot of scripts in parallel, so the CPU is throttling down (and thus a CPU second saved here would count for more during a parallel run; the same work takes more CPU seconds there). We get similar results for t3070: Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 ./t3070-wildmatch.sh Time (mean ± σ): 20.054 s ± 3.967 s [User: 16.003 s, System: 8.286 s] Range (min … max): 11.891 s … 23.671 s 10 runs Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 ./t3070-wildmatch.sh Time (mean ± σ): 12.399 s ± 2.256 s [User: 7.542 s, System: 5.342 s] Range (min … max): 9.606 s … 15.727 s 10 runs Benchmark #3: ./t3070-wildmatch.sh Time (mean ± σ): 10.726 s ± 3.476 s [User: 6.790 s, System: 4.365 s] Range (min … max): 5.444 s … 15.376 s 10 runs Summary './t3070-wildmatch.sh' ran 1.16 ± 0.43 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=0 ./t3070-wildmatch.sh' 1.87 ± 0.71 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 ./t3070-wildmatch.sh' Again, we get almost a 2x speedup disabling these. In this case, there are no tests not covered by the script's "default to disable" behavior, so the second two benchmarks should be the same (and while they do differ, you can see the variance is quite high but they're within one standard deviation). So it seems like for these two scripts, at least, disabling the extra checks is a reasonable tradeoff. Sadly, the overall runtime of "make test" on my system doesn't get much faster. But that's because we're mostly limited by the cost of the single biggest test. Here are the top-5 tests by wall-clock time from a parallel run, before my patch: 57.9192368984222 t9001-send-email.sh 45.6329638957977 t0027-auto-crlf.sh 32.5278220176697 t3070-wildmatch.sh 22.2701289653778 t7610-mergetool.sh 20.8635759353638 t1701-racy-split-index.sh And after: 57.1476998329163 t9001-send-email.sh 33.776211977005 t0027-auto-crlf.sh 21.3116669654846 t7610-mergetool.sh 20.7748689651489 t1701-racy-split-index.sh 19.6957249641418 t7112-reset-submodule.sh We dropped 12s from t0027, and t3070 dropped off our list entirely at around 16s. In both cases we're bound by t9001, but its slowness is due to the actual tests, so we'll have to deal with it in a different way. But this reduces overall CPU, and means that dealing with t9001 (by improving the speed of send-email or splitting it apart) will let us reduce our overall runtime even on multi-core machines. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

shorten_unambiguous_ref() returns an allocated string. We have to track it separately from the const refname. 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 microsoft#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14 microsoft#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c microsoft#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9 microsoft#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14 microsoft#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9 microsoft#6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#10 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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>

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 microsoft#1 0x9a7108 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14 microsoft#2 0x8add6b in expand_ref /home/ahunt/oss-fuzz/git/refs.c:670:12 microsoft#3 0x8ad777 in repo_dwim_ref /home/ahunt/oss-fuzz/git/refs.c:644:22 microsoft#4 0x6394af in dwim_ref /home/ahunt/oss-fuzz/git/./refs.h:162:9 microsoft#5 0x637e5c in cmd_reset /home/ahunt/oss-fuzz/git/builtin/reset.c:426:4 microsoft#6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#10 0x69c5ce in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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>

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 microsoft#1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8 microsoft#2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9 microsoft#3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8 microsoft#4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10 microsoft#5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4 microsoft#6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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 microsoft#1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8 microsoft#2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9 microsoft#3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8 microsoft#4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21 microsoft#5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17 microsoft#6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#11 0x7f8fef0c2349 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 microsoft#1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8 microsoft#2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9 microsoft#3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8 microsoft#4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10 microsoft#5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4 microsoft#6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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 microsoft#1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8 microsoft#2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20 microsoft#3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9 microsoft#4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8 microsoft#5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8 microsoft#6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4 microsoft#7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9 microsoft#8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4 microsoft#9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9 microsoft#10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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 microsoft#1 0x9a71f6 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8 microsoft#2 0x93622d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2 microsoft#3 0x937a73 in strbuf_addch /home/ahunt/oss-fuzz/git/./strbuf.h:231:3 microsoft#4 0x939fcd in strbuf_add_absolute_path /home/ahunt/oss-fuzz/git/strbuf.c:911:4 microsoft#5 0x69d3ce in absolute_pathdup /home/ahunt/oss-fuzz/git/abspath.c:261:2 microsoft#6 0x51c688 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1021:10 microsoft#7 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#8 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#9 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#10 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#11 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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>

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 microsoft#1 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8 microsoft#2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2 microsoft#3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3 microsoft#4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2 microsoft#5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20 microsoft#6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19 microsoft#7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10 microsoft#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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>

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 microsoft#1 0x9a7276 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8 microsoft#2 0x9362ad in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2 microsoft#3 0x936eaa in strbuf_add /home/ahunt/oss-fuzz/git/strbuf.c:295:2 microsoft#4 0x868112 in strbuf_addstr /home/ahunt/oss-fuzz/git/./strbuf.h:304:2 microsoft#5 0x86a8ad in expand_user_path /home/ahunt/oss-fuzz/git/path.c:758:2 microsoft#6 0x720bb1 in git_config_pathname /home/ahunt/oss-fuzz/git/config.c:1287:10 microsoft#7 0x5960e2 in git_init_db_config /home/ahunt/oss-fuzz/git/builtin/init-db.c:161:11 microsoft#8 0x7255b8 in configset_iter /home/ahunt/oss-fuzz/git/config.c:1982:7 microsoft#9 0x7253fc in repo_config /home/ahunt/oss-fuzz/git/config.c:2311:2 microsoft#10 0x725ca7 in git_config /home/ahunt/oss-fuzz/git/config.c:2399:2 microsoft#11 0x593e8d in create_default_files /home/ahunt/oss-fuzz/git/builtin/init-db.c:225:2 microsoft#12 0x5935c6 in init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:449:11 microsoft#13 0x59588e in cmd_init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:714:9 microsoft#14 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#15 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#16 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#17 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#18 0x69c4de in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#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>

preprocess_options() allocates new strings for help messages for OPTION_ALIAS. Therefore we also need to clean those help messages up when freeing the returned options. First introduced in: 7c28058 (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16) The preprocessed options themselves no longer contain any indication that a given option is/was an alias - therefore we add a new flag to indicate former aliases. (An alternative approach would be to look back at the original options to determine which options are aliases - but that seems like a fragile approach. Or we could even look at the alias_groups list - which might be less fragile, but would be slower as it requires nested looping.) As far as I can tell, parse_options() is only ever used once per command, and the help messages are small - hence this leak has very little impact. This leak was found while running t0001. LSAN output can be found below: Direct leak of 65 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 microsoft#1 0x9aae36 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8 microsoft#2 0x939d8d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2 microsoft#3 0x93b936 in strbuf_vaddf /home/ahunt/oss-fuzz/git/strbuf.c:392:3 microsoft#4 0x93b7ff in strbuf_addf /home/ahunt/oss-fuzz/git/strbuf.c:333:2 microsoft#5 0x86747e in preprocess_options /home/ahunt/oss-fuzz/git/parse-options.c:666:3 microsoft#6 0x866ed2 in parse_options /home/ahunt/oss-fuzz/git/parse-options.c:847:17 microsoft#7 0x51c4a7 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:989:9 microsoft#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#12 0x69c9fe in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#13 0x7fdac42d4349 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>

transport_get_remote_refs() can populate the transport struct's remote_refs. transport_disconnect() is already responsible for most of transport's cleanup - therefore we also take care of freeing remote_refs there. There are 2 locations where transport_disconnect() is called before we're done using the returned remote_refs. This patch changes those callsites to only call transport_disconnect() after the returned refs are no longer being used - which is necessary to safely be able to free remote_refs during transport_disconnect(). This commit fixes the following leak which was found while running t0000, but is expected to also fix the same pattern of leak in all locations that use transport_get_remote_refs(): 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 microsoft#1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8 microsoft#2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20 microsoft#3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9 microsoft#4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8 microsoft#5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8 microsoft#6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4 microsoft#7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9 microsoft#8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4 microsoft#9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9 microsoft#10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 microsoft#11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 microsoft#12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 microsoft#13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 microsoft#14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 microsoft#15 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>

versions could be an empty string_list. In that case, versions->items is NULL, and we shouldn't be trying to perform pointer arithmetic with it (as that results in undefined behaviour). Moreover we only use the results of this calculation once when calling QSORT. Therefore we choose to skip creating relevant_entries and call QSORT directly with our manipulated pointers (but only if there's data requiring sorting). This lets us avoid abusing the string_list API, and saves us from having to explain why this abuse is OK. Finally, an assertion is added to make sure that write_tree() is called with a valid offset. This issue has probably existed since: ee4012d (merge-ort: step 2 of tree writing -- function to create tree object, 2020-12-13) But it only started occurring during tests since tests started using merge-ort: f3b964a (Add testing with merge-ort merge strategy, 2021-03-20) For reference - here's the original UBSAN commit that implemented this check, it sounds like this behaviour isn't actually likely to cause any issues (but we might as well fix it regardless): https://reviews.llvm.org/D67122 UBSAN output from t3404 or t5601: merge-ort.c:2669:43: runtime error: applying zero offset to null pointer #0 0x78bb53 in write_tree merge-ort.c:2669:43 microsoft#1 0x7856c9 in process_entries merge-ort.c:3303:2 microsoft#2 0x782317 in merge_ort_nonrecursive_internal merge-ort.c:3744:2 microsoft#3 0x77feef in merge_incore_nonrecursive merge-ort.c:3853:2 microsoft#4 0x6f6a5c in do_recursive_merge sequencer.c:640:3 microsoft#5 0x6f6a5c in do_pick_commit sequencer.c:2221:9 microsoft#6 0x6ef055 in single_pick sequencer.c:4814:9 microsoft#7 0x6ef055 in sequencer_pick_revisions sequencer.c:4867:10 microsoft#8 0x4fb392 in run_sequencer revert.c:225:9 microsoft#9 0x4fa5b0 in cmd_revert revert.c:235:8 microsoft#10 0x42abd7 in run_builtin git.c:453:11 microsoft#11 0x429531 in handle_builtin git.c:704:3 microsoft#12 0x4282fb in run_argv git.c:771:4 microsoft#13 0x4282fb in cmd_main git.c:902:19 microsoft#14 0x524b63 in main common-main.c:52:11 microsoft#15 0x7fc2ca340349 in __libc_start_main (/lib64/libc.so.6+0x24349) microsoft#16 0x4072b9 in _start start.S:120 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior merge-ort.c:2669:43 in Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

limit_list() iterates over the original revs->commits list, and consumes many of its entries via pop_commit. However we might stop iterating over the list early (e.g. if we realise that the rest of the list is uninteresting). If we do stop iterating early, list will be pointing to the unconsumed portion of revs->commits - and we need to free this list to avoid a leak. (revs->commits itself will be an invalid pointer: it will have been free'd during the first pop_commit.) However the list pointer is later reused to iterate over our new list, but only for the limiting_can_increase_treesame() branch. We therefore need to introduce a new variable for that branch - and while we're here we can rename the original list to original_list as that makes its purpose more obvious. This leak was found while running t0090. It's not likely to be very impactful, but it can happen quite early during some checkout invocations, and hence seems to be worth fixing: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9ac084 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9ac05a in xmalloc wrapper.c:62:9 microsoft#3 0x7175d6 in commit_list_insert commit.c:540:33 microsoft#4 0x71800f in commit_list_insert_by_date commit.c:604:9 microsoft#5 0x8f8d2e in process_parents revision.c:1128:5 microsoft#6 0x8f2f2c in limit_list revision.c:1418:7 microsoft#7 0x8f210e in prepare_revision_walk revision.c:3577:7 microsoft#8 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6 microsoft#9 0x512f05 in switch_branches builtin/checkout.c:1250:3 microsoft#10 0x50f8de in checkout_branch builtin/checkout.c:1646:9 microsoft#11 0x50ba12 in checkout_main builtin/checkout.c:2003:9 microsoft#12 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 microsoft#13 0x4cd91d in run_builtin git.c:467:11 microsoft#14 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#15 0x4ccf47 in run_argv git.c:808:4 microsoft#16 0x4caf49 in cmd_main git.c:939:19 microsoft#17 0x69dc0e in main common-main.c:52:11 microsoft#18 0x7faaabd0e349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 48 byte(s) in 3 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9ac084 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9ac05a in xmalloc wrapper.c:62:9 microsoft#3 0x717de6 in commit_list_append commit.c:1609:35 microsoft#4 0x8f1f9b in prepare_revision_walk revision.c:3554:12 microsoft#5 0x514170 in orphaned_commit_warning builtin/checkout.c:1185:6 microsoft#6 0x512f05 in switch_branches builtin/checkout.c:1250:3 microsoft#7 0x50f8de in checkout_branch builtin/checkout.c:1646:9 microsoft#8 0x50ba12 in checkout_main builtin/checkout.c:2003:9 microsoft#9 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 microsoft#10 0x4cd91d in run_builtin git.c:467:11 microsoft#11 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#12 0x4ccf47 in run_argv git.c:808:4 microsoft#13 0x4caf49 in cmd_main git.c:939:19 microsoft#14 0x69dc0e in main common-main.c:52:11 microsoft#15 0x7faaabd0e349 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>

common_prefix() returns a new string, which we store in max_prefix - this string needs to be freed to avoid a leak. This leak is happening in cmd_ls_files, hence is of no real consequence - an UNLEAK would be just as good, but we might as well free the string properly. Leak found while running t0002, see output below: Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9ab1b4 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9ab248 in do_xmallocz wrapper.c:75:8 microsoft#3 0x9ab22a in xmallocz wrapper.c:83:9 microsoft#4 0x9ab2d7 in xmemdupz wrapper.c:99:16 microsoft#5 0x78d6a4 in common_prefix dir.c:191:15 microsoft#6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16 microsoft#7 0x4cd92d in run_builtin git.c:453:11 microsoft#8 0x4cb5fa in handle_builtin git.c:704:3 microsoft#9 0x4ccf57 in run_argv git.c:771:4 microsoft#10 0x4caf49 in cmd_main git.c:902:19 microsoft#11 0x69ce2e in main common-main.c:52:11 microsoft#12 0x7f64d4d94349 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>

fill_bloom_key() allocates memory into bloom_key, we need to clean that up once the key is no longer needed. This leak was found while running t0002-t0099. Although this leak is happening in code being called from a test-helper, the same code is also used in various locations around git, and can therefore happen during normal usage too. Gabor's analysis shows that peak-memory usage during 'git commit-graph write' is reduced on the order of 10% for a selection of larger repos (along with an even larger reduction if we override modified path bloom filter limits): https://lore.kernel.org/git/20210411072651.GF2947267@szeder.dev/ LSAN output: Direct leak of 308 byte(s) in 11 object(s) allocated from: #0 0x49a5e2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 microsoft#1 0x6f4032 in xcalloc wrapper.c:140:8 microsoft#2 0x4f2905 in fill_bloom_key bloom.c:137:28 microsoft#3 0x4f34c1 in get_or_compute_bloom_filter bloom.c:284:4 microsoft#4 0x4cb484 in get_bloom_filter_for_commit t/helper/test-bloom.c:43:11 microsoft#5 0x4cb072 in cmd__bloom t/helper/test-bloom.c:97:3 microsoft#6 0x4ca7ef in cmd_main t/helper/test-tool.c:121:11 microsoft#7 0x4caace in main common-main.c:52:11 microsoft#8 0x7f798af95349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 308 byte(s) leaked in 11 allocation(s). Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

real_ref was previously populated by dwim_ref(), which allocates new memory. We need to make sure to free real_ref when discarding it. (real_ref is already being freed at the end of create_branch() - but if we discard it early then it will leak.) This fixes the following leak found while running t0002-t0099: Direct leak of 5 byte(s) in 1 object(s) allocated from: #0 0x486954 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 microsoft#1 0xdd6484 in xstrdup wrapper.c:29:14 microsoft#2 0xc0f658 in expand_ref refs.c:671:12 microsoft#3 0xc0ecf1 in repo_dwim_ref refs.c:644:22 microsoft#4 0x8b1184 in dwim_ref ./refs.h:162:9 microsoft#5 0x8b0b02 in create_branch branch.c:284:10 microsoft#6 0x550cbb in update_refs_for_switch builtin/checkout.c:1046:4 microsoft#7 0x54e275 in switch_branches builtin/checkout.c:1274:2 microsoft#8 0x548828 in checkout_branch builtin/checkout.c:1668:9 microsoft#9 0x541306 in checkout_main builtin/checkout.c:2025:9 microsoft#10 0x5395fa in cmd_checkout builtin/checkout.c:2077:8 microsoft#11 0x4d02a8 in run_builtin git.c:467:11 microsoft#12 0x4cbfe9 in handle_builtin git.c:719:3 microsoft#13 0x4cf04f in run_argv git.c:808:4 microsoft#14 0x4cb85a in cmd_main git.c:939:19 microsoft#15 0x820cf6 in main common-main.c:52:11 microsoft#16 0x7f30bd9dd349 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>

prefix_filename() returns newly allocated memory, and strbuf_addstr() doesn't take ownership of its inputs. Therefore we have to make sure to store and free prefix_filename()'s result. As this leak is in cmd_bugreport(), we could just as well UNLEAK the prefix - but there's no good reason not to just free it properly. This leak was found while running t0091, see output below: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 microsoft#1 0x9acc66 in xrealloc wrapper.c:126:8 microsoft#2 0x93baed in strbuf_grow strbuf.c:98:2 microsoft#3 0x93c6ea in strbuf_add strbuf.c:295:2 microsoft#4 0x69f162 in strbuf_addstr ./strbuf.h:304:2 microsoft#5 0x69f083 in prefix_filename abspath.c:277:2 microsoft#6 0x4fb275 in cmd_bugreport builtin/bugreport.c:146:9 microsoft#7 0x4cd91d in run_builtin git.c:467:11 microsoft#8 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#9 0x4ccf47 in run_argv git.c:808:4 microsoft#10 0x4caf49 in cmd_main git.c:939:19 microsoft#11 0x69df9e in main common-main.c:52:11 microsoft#12 0x7f523a987349 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>

parse_pathspec() allocates new memory into pathspec, therefore we need to free it when we're done. An UNLEAK would probably be just as good here - but clear_pathspec() is not much more work so we might as well use it. check_ignore() is either called once directly from cmd_check_ignore() (in which case the leak really doesnt matter), or it can be called multiple times in a loop from check_ignore_stdin_paths(), in which case we're potentially leaking multiple times - but even in this scenario the leak is so small as to have no real consequence. Found while running t0008: Direct leak of 112 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9aca44 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9aca1a in xmalloc wrapper.c:62:9 microsoft#3 0x873c17 in parse_pathspec pathspec.c:582:2 microsoft#4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 microsoft#5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 microsoft#6 0x4cd91d in run_builtin git.c:467:11 microsoft#7 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#8 0x4ccf47 in run_argv git.c:808:4 microsoft#9 0x4caf49 in cmd_main git.c:939:19 microsoft#10 0x69e43e in main common-main.c:52:11 microsoft#11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 65 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 microsoft#1 0x9acc46 in xrealloc wrapper.c:126:8 microsoft#2 0x93baed in strbuf_grow strbuf.c:98:2 microsoft#3 0x93d696 in strbuf_vaddf strbuf.c:392:3 microsoft#4 0x9400c6 in xstrvfmt strbuf.c:979:2 microsoft#5 0x940253 in xstrfmt strbuf.c:989:8 microsoft#6 0x92b72a in prefix_path_gently setup.c:115:15 microsoft#7 0x87442d in init_pathspec_item pathspec.c:439:11 microsoft#8 0x873cef in parse_pathspec pathspec.c:589:3 microsoft#9 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 microsoft#10 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 microsoft#11 0x4cd91d in run_builtin git.c:467:11 microsoft#12 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#13 0x4ccf47 in run_argv git.c:808:4 microsoft#14 0x4caf49 in cmd_main git.c:939:19 microsoft#15 0x69e43e in main common-main.c:52:11 microsoft#16 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 2 byte(s) in 1 object(s) allocated from: #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 microsoft#1 0x9ac9e8 in xstrdup wrapper.c:29:14 microsoft#2 0x874542 in init_pathspec_item pathspec.c:468:20 microsoft#3 0x873cef in parse_pathspec pathspec.c:589:3 microsoft#4 0x503eb8 in check_ignore builtin/check-ignore.c:90:2 microsoft#5 0x5038af in cmd_check_ignore builtin/check-ignore.c:190:17 microsoft#6 0x4cd91d in run_builtin git.c:467:11 microsoft#7 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#8 0x4ccf47 in run_argv git.c:808:4 microsoft#9 0x4caf49 in cmd_main git.c:939:19 microsoft#10 0x69e43e in main common-main.c:52:11 microsoft#11 0x7f18bb0dd349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 179 byte(s) leaked in 3 allocation(s). Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

add_pending_object() populates rev.pending, we need to take care of clearing it once we're done. This code is run close to the end of a checkout, therefore this leak seems like it would have very little impact. See also LSAN output from t0020 below: Direct leak of 2048 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 microsoft#1 0x9acc46 in xrealloc wrapper.c:126:8 microsoft#2 0x83e3a3 in add_object_array_with_path object.c:337:3 microsoft#3 0x8f672a in add_pending_object_with_path revision.c:329:2 microsoft#4 0x8eaeab in add_pending_object_with_mode revision.c:336:2 microsoft#5 0x8eae9d in add_pending_object revision.c:342:2 microsoft#6 0x5154a0 in show_local_changes builtin/checkout.c:602:2 microsoft#7 0x513b00 in merge_working_tree builtin/checkout.c:979:3 microsoft#8 0x512cb3 in switch_branches builtin/checkout.c:1242:9 microsoft#9 0x50f8de in checkout_branch builtin/checkout.c:1646:9 microsoft#10 0x50ba12 in checkout_main builtin/checkout.c:2003:9 microsoft#11 0x5086c0 in cmd_checkout builtin/checkout.c:2055:8 microsoft#12 0x4cd91d in run_builtin git.c:467:11 microsoft#13 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#14 0x4ccf47 in run_argv git.c:808:4 microsoft#15 0x4caf49 in cmd_main git.c:939:19 microsoft#16 0x69e43e in main common-main.c:52:11 microsoft#17 0x7f5dd1d50349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 2048 byte(s) leaked in 1 allocation(s). Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

mailinfo.p_hdr_info/s_hdr_info are null-terminated lists of strbuf's, with entries pointing either to NULL or an allocated strbuf. Therefore we need to free those strbuf's (and not just the data they contain) whenever we're done with a given entry. (See handle_header() where those new strbufs are malloc'd.) Once we no longer need the list (and not just its entries) we can switch over to strbuf_list_free() instead of manually iterating over the list, which takes care of those additional details for us. We can only do this in clear_mailinfo() - in handle_commit_message() we are only clearing the array contents but want to reuse the array itself, hence we can't use strbuf_list_free() there. However, strbuf_list_free() cannot handle a NULL input, and the lists we are freeing might be NULL. Therefore we add a NULL check in strbuf_list_free() to make it safe to use with a NULL input (which is a pattern used by some of the other *_free() functions around git). Leak output from t0023: Direct leak of 72 byte(s) in 3 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9ac9f4 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9ac9ca in xmalloc wrapper.c:62:9 microsoft#3 0x7f6cf7 in handle_header mailinfo.c:205:10 microsoft#4 0x7f5abf in check_header mailinfo.c:583:4 microsoft#5 0x7f5524 in mailinfo mailinfo.c:1197:3 microsoft#6 0x4dcc95 in parse_mail builtin/am.c:1167:6 microsoft#7 0x4d9070 in am_run builtin/am.c:1732:12 microsoft#8 0x4d5b7a in cmd_am builtin/am.c:2398:3 microsoft#9 0x4cd91d in run_builtin git.c:467:11 microsoft#10 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#11 0x4ccf47 in run_argv git.c:808:4 microsoft#12 0x4caf49 in cmd_main git.c:939:19 microsoft#13 0x69e43e in main common-main.c:52:11 microsoft#14 0x7fc1fadfa349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s). Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

sorting might be a list allocated in ref_default_sorting() (in this case it's a fixed single item list, which has nevertheless been xcalloc'd), or it might be a list allocated in parse_opt_ref_sorting(). In either case we could free these lists - but instead we UNLEAK as we're at the end of cmd_for_each_ref. (There's no existing implementation of clear_ref_sorting(), and writing a loop to free the list seems more trouble than it's worth.) filter.with_commit/no_commit are populated via OPT_CONTAINS/OPT_NO_CONTAINS, both of which create new entries via parse_opt_commits(), and also need to be free'd or UNLEAK'd. Because free_commit_list() already exists, we choose to use that over an UNLEAK. LSAN output from t0041: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 microsoft#1 0x9ac252 in xcalloc wrapper.c:140:8 microsoft#2 0x8a4a55 in ref_default_sorting ref-filter.c:2486:32 microsoft#3 0x56c6b1 in cmd_for_each_ref builtin/for-each-ref.c:72:13 microsoft#4 0x4cd91d in run_builtin git.c:467:11 microsoft#5 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#6 0x4ccf47 in run_argv git.c:808:4 microsoft#7 0x4caf49 in cmd_main git.c:939:19 microsoft#8 0x69dabe in main common-main.c:52:11 microsoft#9 0x7f2bdc570349 in __libc_start_main (/lib64/libc.so.6+0x24349) Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9abf54 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9abf2a in xmalloc wrapper.c:62:9 microsoft#3 0x717486 in commit_list_insert commit.c:540:33 microsoft#4 0x8644cf in parse_opt_commits parse-options-cb.c:98:2 microsoft#5 0x869bb5 in get_value parse-options.c:181:11 microsoft#6 0x8677dc in parse_long_opt parse-options.c:378:10 microsoft#7 0x8659bd in parse_options_step parse-options.c:817:11 microsoft#8 0x867fcd in parse_options parse-options.c:870:10 microsoft#9 0x56c62b in cmd_for_each_ref builtin/for-each-ref.c:59:2 microsoft#10 0x4cd91d in run_builtin git.c:467:11 microsoft#11 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#12 0x4ccf47 in run_argv git.c:808:4 microsoft#13 0x4caf49 in cmd_main git.c:939:19 microsoft#14 0x69dabe in main common-main.c:52:11 microsoft#15 0x7f2bdc570349 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>

options.git_format_patch_opt can be populated during cmd_rebase's setup, and will therefore leak on return. Although we could just UNLEAK all of options, we choose to strbuf_release() the individual member, which matches the existing pattern (where we're freeing invidual members of options). Leak found when running t0021: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 microsoft#1 0x9ac296 in xrealloc wrapper.c:126:8 microsoft#2 0x93b13d in strbuf_grow strbuf.c:98:2 microsoft#3 0x93bd3a in strbuf_add strbuf.c:295:2 microsoft#4 0x60ae92 in strbuf_addstr strbuf.h:304:2 microsoft#5 0x605f17 in cmd_rebase builtin/rebase.c:1759:3 microsoft#6 0x4cd91d in run_builtin git.c:467:11 microsoft#7 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#8 0x4ccf47 in run_argv git.c:808:4 microsoft#9 0x4caf49 in cmd_main git.c:939:19 microsoft#10 0x69dbfe in main common-main.c:52:11 microsoft#11 0x7f66dae91349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s). Signed-off-by: Andrzej Hunt <ajrhunt@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

parse_pathspec() populates pathspec, hence we need to clear it once it's no longer needed. seen is xcalloc'd within the same function and likewise needs to be freed once its no longer needed. cmd_rm() has multiple early returns, therefore we need to clear or free as soon as this data is no longer needed, as opposed to doing a cleanup at the end. LSAN output from t0020: Direct leak of 112 byte(s) in 1 object(s) allocated from: #0 0x49a85d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 microsoft#1 0x9ac0a4 in do_xmalloc wrapper.c:41:8 microsoft#2 0x9ac07a in xmalloc wrapper.c:62:9 microsoft#3 0x873277 in parse_pathspec pathspec.c:582:2 microsoft#4 0x646ffa in cmd_rm builtin/rm.c:266:2 microsoft#5 0x4cd91d in run_builtin git.c:467:11 microsoft#6 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#7 0x4ccf47 in run_argv git.c:808:4 microsoft#8 0x4caf49 in cmd_main git.c:939:19 microsoft#9 0x69dc0e in main common-main.c:52:11 microsoft#10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 65 byte(s) in 1 object(s) allocated from: #0 0x49ab79 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 microsoft#1 0x9ac2a6 in xrealloc wrapper.c:126:8 microsoft#2 0x93b14d in strbuf_grow strbuf.c:98:2 microsoft#3 0x93ccf6 in strbuf_vaddf strbuf.c:392:3 microsoft#4 0x93f726 in xstrvfmt strbuf.c:979:2 microsoft#5 0x93f8b3 in xstrfmt strbuf.c:989:8 microsoft#6 0x92ad8a in prefix_path_gently setup.c:115:15 microsoft#7 0x873a8d in init_pathspec_item pathspec.c:439:11 microsoft#8 0x87334f in parse_pathspec pathspec.c:589:3 microsoft#9 0x646ffa in cmd_rm builtin/rm.c:266:2 microsoft#10 0x4cd91d in run_builtin git.c:467:11 microsoft#11 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#12 0x4ccf47 in run_argv git.c:808:4 microsoft#13 0x4caf49 in cmd_main git.c:939:19 microsoft#14 0x69dc0e in main common-main.c:52:11 microsoft#15 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 15 byte(s) in 1 object(s) allocated from: #0 0x486834 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 microsoft#1 0x9ac048 in xstrdup wrapper.c:29:14 microsoft#2 0x873ba2 in init_pathspec_item pathspec.c:468:20 microsoft#3 0x87334f in parse_pathspec pathspec.c:589:3 microsoft#4 0x646ffa in cmd_rm builtin/rm.c:266:2 microsoft#5 0x4cd91d in run_builtin git.c:467:11 microsoft#6 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#7 0x4ccf47 in run_argv git.c:808:4 microsoft#8 0x4caf49 in cmd_main git.c:939:19 microsoft#9 0x69dc0e in main common-main.c:52:11 microsoft#10 0x7f948825b349 in __libc_start_main (/lib64/libc.so.6+0x24349) Direct leak of 1 byte(s) in 1 object(s) allocated from: #0 0x49a9d2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 microsoft#1 0x9ac392 in xcalloc wrapper.c:140:8 microsoft#2 0x647108 in cmd_rm builtin/rm.c:294:9 microsoft#3 0x4cd91d in run_builtin git.c:467:11 microsoft#4 0x4cb5f3 in handle_builtin git.c:719:3 microsoft#5 0x4ccf47 in run_argv git.c:808:4 microsoft#6 0x4caf49 in cmd_main git.c:939:19 microsoft#7 0x69dbfe in main common-main.c:52:11 microsoft#8 0x7f4fac1b0349 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>

In a sparse index it is possible for the tree that is being verified to be freed while it is being verified. This happens when the index is sparse but the cache tree is not and index_name_pos() looks up a path from the cache tree that is a descendant of a sparse index entry. That triggers a call to ensure_full_index() which frees the cache tree that is being verified. Carrying on trying to verify the tree after this results in a use-after-free bug. Instead restart the verification if a sparse index is converted to a full index. This bug is triggered by a call to reset_head() in "git rebase --apply". Thanks to RenÃ© Scharfe and Derrick Stolee for their help analyzing the problem. ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080 READ of size 4 at 0x606000001b20 thread T0 #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863 #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 #5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d) 0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58) freed by thread T0 here: #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127 #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35 #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310 #6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588 #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850 #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 #12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) previously allocated by thread T0 here: #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140 #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17 #3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763 #4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 #5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 #6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779 #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85 #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

When I was playing around with trace2 data and creating flamegraphs, I tried a `git fetch` call to see how the `git-remote-https` command would show up. What I didn't expect was an `ensure_full_index()` region! It turns out that `git fetch` and `git pull` need to check the index for a `.gitmodules` file to see if it should recurse into any submodules. Here is the stack trace from a debugger: ``` #0 ensure_full_index (istate=0x555555ac1c80 <the_index>) at sparse-index.c:404 #1 0x000055555571a979 in do_read_index (istate=istate@entry=0x555555ac1c80 <the_index>, path=path@entry=0x555555ad7b90 ".git/index", must_exist=must_exist@entry=0) at read-cache.c:2386 #2 0x000055555571eb7d in do_read_index (must_exist=0, path=0x555555ad7b90 ".git/index", istate=0x555555ac1c80 <the_index>) at hash.h:244 #3 read_index_from (istate=0x555555ac1c80 <the_index>, path=0x555555ad7b90 ".git/index", gitdir=0x555555ad7b30 ".git") at read-cache.c:2426 #4 0x000055555573f4c2 in repo_read_index (repo=repo@entry=0x555555ac1da0 <the_repo>) at repository.c:286 #5 0x00005555556f14d0 in get_oid_with_context_1 (repo=repo@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", flags=flags@entry=0, prefix=prefix@entry=0x0, oid=oid@entry=0x7fffffffdb00, oc=oc@entry=0x7fffffffda70) at object-name.c:1850 #6 0x00005555556f1f53 in get_oid_with_context (oc=0x7fffffffda70, oid=0x7fffffffdb00, flags=0, str=0x55555582c022 ":.gitmodules", repo=0x555555ac1da0 <the_repo>) at object-name.c:1947 #7 repo_get_oid (r=r@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", oid=oid@entry=0x7fffffffdb00) at object-name.c:1603 #8 0x000055555577330f in config_from_gitmodules (fn=fn@entry=0x555555773460 <gitmodules_fetch_config>, repo=0x555555ac1da0 <the_repo>, data=data@entry=0x7fffffffdb60) at submodule-config.c:650 #9 0x000055555577462d in config_from_gitmodules (data=0x7fffffffdb60, repo=<optimized out>, fn=0x555555773460 <gitmodules_fetch_config>) at submodule-config.c:638 #10 fetch_config_from_gitmodules (max_children=<optimized out>, recurse_submodules=<optimized out>) at submodule-config.c:800 #11 0x00005555555b9e41 in cmd_fetch (argc=1, argv=0x7fffffffe090, prefix=0x0) at builtin/fetch.c:1999 #12 0x0000555555573ff6 in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:528 #13 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:785 #14 0x000055555557528c in run_argv (argv=0x7fffffffddf0, argcp=0x7fffffffddfc) at git.c:857 #15 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:993 #16 0x0000555555573ac8 in main (argc=3, argv=0x7fffffffe088) at common-main.c:52 ``` The operations these commands use are guarded by items such as `index_name_pos()` and others. Since the `.gitmodules` file is always at root, we would not need to expand, anyway.

In a sparse index it is possible for the tree that is being verified to be freed while it is being verified. This happens when the index is sparse but the cache tree is not and index_name_pos() looks up a path from the cache tree that is a descendant of a sparse index entry. That triggers a call to ensure_full_index() which frees the cache tree that is being verified. Carrying on trying to verify the tree after this results in a use-after-free bug. Instead restart the verification if a sparse index is converted to a full index. This bug is triggered by a call to reset_head() in "git rebase --apply". Thanks to René Scharfe and Derrick Stolee for their help analyzing the problem. ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080 READ of size 4 at 0x606000001b20 thread T0 #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863 #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 microsoft#5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 microsoft#6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 microsoft#7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 microsoft#8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 microsoft#9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 microsoft#10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 microsoft#11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 microsoft#12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 microsoft#13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) microsoft#14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d) 0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58) freed by thread T0 here: #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127 #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35 microsoft#2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 microsoft#3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 microsoft#4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 microsoft#5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310 microsoft#6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588 microsoft#7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850 microsoft#8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 microsoft#11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 microsoft#12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 microsoft#13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 microsoft#14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 microsoft#15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 microsoft#16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 microsoft#17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 microsoft#18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 microsoft#19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 microsoft#20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) previously allocated by thread T0 here: #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140 microsoft#2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17 microsoft#3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763 microsoft#4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 microsoft#5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 microsoft#6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779 microsoft#7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85 microsoft#8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 microsoft#9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 microsoft#10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 microsoft#11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 microsoft#12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 microsoft#13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 microsoft#14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

In a sparse index it is possible for the tree that is being verified to be freed while it is being verified. This happens when the index is sparse but the cache tree is not and index_name_pos() looks up a path from the cache tree that is a descendant of a sparse index entry. That triggers a call to ensure_full_index() which frees the cache tree that is being verified. Carrying on trying to verify the tree after this results in a use-after-free bug. Instead restart the verification if a sparse index is converted to a full index. This bug is triggered by a call to reset_head() in "git rebase --apply". Thanks to RenÃ© Scharfe and Derrick Stolee for their help analyzing the problem. ==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080 READ of size 4 at 0x606000001b20 thread T0 #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863 #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 #5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d) 0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58) freed by thread T0 here: #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127 #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35 #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31 #5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310 #6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588 #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850 #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840 #11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910 #12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250 #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87 #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) previously allocated by thread T0 here: #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140 #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17 #3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763 #4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 #5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764 #6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779 #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85 #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074 #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461 #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714 #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781 #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912 #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52 #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

When I was playing around with trace2 data and creating flamegraphs, I tried a `git fetch` call to see how the `git-remote-https` command would show up. What I didn't expect was an `ensure_full_index()` region! It turns out that `git fetch` and `git pull` need to check the index for a `.gitmodules` file to see if it should recurse into any submodules. Here is the stack trace from a debugger: ``` #0 ensure_full_index (istate=0x555555ac1c80 <the_index>) at sparse-index.c:404 #1 0x000055555571a979 in do_read_index (istate=istate@entry=0x555555ac1c80 <the_index>, path=path@entry=0x555555ad7b90 ".git/index", must_exist=must_exist@entry=0) at read-cache.c:2386 #2 0x000055555571eb7d in do_read_index (must_exist=0, path=0x555555ad7b90 ".git/index", istate=0x555555ac1c80 <the_index>) at hash.h:244 #3 read_index_from (istate=0x555555ac1c80 <the_index>, path=0x555555ad7b90 ".git/index", gitdir=0x555555ad7b30 ".git") at read-cache.c:2426 #4 0x000055555573f4c2 in repo_read_index (repo=repo@entry=0x555555ac1da0 <the_repo>) at repository.c:286 #5 0x00005555556f14d0 in get_oid_with_context_1 (repo=repo@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", flags=flags@entry=0, prefix=prefix@entry=0x0, oid=oid@entry=0x7fffffffdb00, oc=oc@entry=0x7fffffffda70) at object-name.c:1850 #6 0x00005555556f1f53 in get_oid_with_context (oc=0x7fffffffda70, oid=0x7fffffffdb00, flags=0, str=0x55555582c022 ":.gitmodules", repo=0x555555ac1da0 <the_repo>) at object-name.c:1947 #7 repo_get_oid (r=r@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", oid=oid@entry=0x7fffffffdb00) at object-name.c:1603 #8 0x000055555577330f in config_from_gitmodules (fn=fn@entry=0x555555773460 <gitmodules_fetch_config>, repo=0x555555ac1da0 <the_repo>, data=data@entry=0x7fffffffdb60) at submodule-config.c:650 #9 0x000055555577462d in config_from_gitmodules (data=0x7fffffffdb60, repo=<optimized out>, fn=0x555555773460 <gitmodules_fetch_config>) at submodule-config.c:638 #10 fetch_config_from_gitmodules (max_children=<optimized out>, recurse_submodules=<optimized out>) at submodule-config.c:800 #11 0x00005555555b9e41 in cmd_fetch (argc=1, argv=0x7fffffffe090, prefix=0x0) at builtin/fetch.c:1999 #12 0x0000555555573ff6 in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:528 #13 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:785 #14 0x000055555557528c in run_argv (argv=0x7fffffffddf0, argcp=0x7fffffffddfc) at git.c:857 #15 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:993 #16 0x0000555555573ac8 in main (argc=3, argv=0x7fffffffe088) at common-main.c:52 ``` The operations these commands use are guarded by items such as `index_name_pos()` and others. Since the `.gitmodules` file is always at root, we would not need to expand, anyway.

When I was playing around with trace2 data and creating flamegraphs, I tried a `git fetch` call to see how the `git-remote-https` command would show up. What I didn't expect was an `ensure_full_index()` region! It turns out that `git fetch` and `git pull` need to check the index for a `.gitmodules` file to see if it should recurse into any submodules. Here is the stack trace from a debugger: ``` #0 ensure_full_index (istate=0x555555ac1c80 <the_index>) at sparse-index.c:404 microsoft#1 0x000055555571a979 in do_read_index (istate=istate@entry=0x555555ac1c80 <the_index>, path=path@entry=0x555555ad7b90 ".git/index", must_exist=must_exist@entry=0) at read-cache.c:2386 microsoft#2 0x000055555571eb7d in do_read_index (must_exist=0, path=0x555555ad7b90 ".git/index", istate=0x555555ac1c80 <the_index>) at hash.h:244 microsoft#3 read_index_from (istate=0x555555ac1c80 <the_index>, path=0x555555ad7b90 ".git/index", gitdir=0x555555ad7b30 ".git") at read-cache.c:2426 microsoft#4 0x000055555573f4c2 in repo_read_index (repo=repo@entry=0x555555ac1da0 <the_repo>) at repository.c:286 microsoft#5 0x00005555556f14d0 in get_oid_with_context_1 (repo=repo@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", flags=flags@entry=0, prefix=prefix@entry=0x0, oid=oid@entry=0x7fffffffdb00, oc=oc@entry=0x7fffffffda70) at object-name.c:1850 microsoft#6 0x00005555556f1f53 in get_oid_with_context (oc=0x7fffffffda70, oid=0x7fffffffdb00, flags=0, str=0x55555582c022 ":.gitmodules", repo=0x555555ac1da0 <the_repo>) at object-name.c:1947 microsoft#7 repo_get_oid (r=r@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", oid=oid@entry=0x7fffffffdb00) at object-name.c:1603 microsoft#8 0x000055555577330f in config_from_gitmodules (fn=fn@entry=0x555555773460 <gitmodules_fetch_config>, repo=0x555555ac1da0 <the_repo>, data=data@entry=0x7fffffffdb60) at submodule-config.c:650 microsoft#9 0x000055555577462d in config_from_gitmodules (data=0x7fffffffdb60, repo=<optimized out>, fn=0x555555773460 <gitmodules_fetch_config>) at submodule-config.c:638 microsoft#10 fetch_config_from_gitmodules (max_children=<optimized out>, recurse_submodules=<optimized out>) at submodule-config.c:800 microsoft#11 0x00005555555b9e41 in cmd_fetch (argc=1, argv=0x7fffffffe090, prefix=0x0) at builtin/fetch.c:1999 microsoft#12 0x0000555555573ff6 in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:528 microsoft#13 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:785 microsoft#14 0x000055555557528c in run_argv (argv=0x7fffffffddf0, argcp=0x7fffffffddfc) at git.c:857 microsoft#15 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:993 microsoft#16 0x0000555555573ac8 in main (argc=3, argv=0x7fffffffe088) at common-main.c:52 ``` The operations these commands use are guarded by items such as `index_name_pos()` and others. Since the `.gitmodules` file is always at root, we would not need to expand, anyway.

When fetching packfiles, we write a bunch of lockfiles for the packfiles we're writing into the repository. In order to not leave behind any cruft in case we exit or receive a signal, we register both an exit handler as well as signal handlers for common signals like SIGINT. These handlers will then unlink the locks and free the data structure tracking them. We have observed a deadlock in this logic though: (gdb) bt #0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95 microsoft#1 0x00007f4932bea2cd in _int_free (av=0x7f4932f2eb20 <main_arena>, p=0x3e3e4200, have_lock=0) at malloc.c:3969 microsoft#2 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975 microsoft#3 0x0000000000662ab1 in string_list_clear () microsoft#4 0x000000000044f5bc in unlock_pack_on_signal () microsoft#5 <signal handler called> microsoft#6 _int_free (av=0x7f4932f2eb20 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:4024 microsoft#7 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975 microsoft#8 0x000000000065afd5 in strbuf_release () microsoft#9 0x000000000066ddb9 in delete_tempfile () microsoft#10 0x0000000000610d0b in files_transaction_cleanup.isra () microsoft#11 0x0000000000611718 in files_transaction_abort () microsoft#12 0x000000000060d2ef in ref_transaction_abort () microsoft#13 0x000000000060d441 in ref_transaction_prepare () microsoft#14 0x000000000060e0b5 in ref_transaction_commit () microsoft#15 0x00000000004511c2 in fetch_and_consume_refs () microsoft#16 0x000000000045279a in cmd_fetch () microsoft#17 0x0000000000407c48 in handle_builtin () microsoft#18 0x0000000000408df2 in cmd_main () microsoft#19 0x00000000004078b5 in main () The process was killed with a signal, which caused the signal handler to kick in and try free the data structures after we have unlinked the locks. It then deadlocks while calling free(3P). The root cause of this is that it is not allowed to call certain functions in async-signal handlers, as specified by signal-safety(7). Next to most I/O functions, this list of disallowed functions also includes memory-handling functions like malloc(3P) and free(3P) because they may not be reentrant. As a result, if we execute such functions in the signal handler, then they may operate on inconistent state and fail in unexpected ways. Fix this bug by not calling non-async-signal-safe functions when running in the signal handler. We're about to re-raise the signal anyway and will thus exit, so it's not much of a problem to keep the string list of lockfiles untouched. Note that it's fine though to call unlink(2), so we'll still clean up the lockfiles correctly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Reviewed-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

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 microsoft#2 0x5995fa in parse_date_format date.c:991:24 microsoft#3 0x4d2056 in show_dates t/helper/test-date.c:39:2 microsoft#4 0x4d174a in cmd__date t/helper/test-date.c:116:3 microsoft#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11 microsoft#6 0x4cd1e3 in main common-main.c:52:11 microsoft#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16 microsoft#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 microsoft#2 0x5cb164 in xstrdup wrapper.c:29:14 microsoft#3 0x495ee9 in parse_date_format date.c:991:24 microsoft#4 0x453aac in show_dates t/helper/test-date.c:39:2 microsoft#5 0x453782 in cmd__date t/helper/test-date.c:116:3 microsoft#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11 microsoft#7 0x451f1e in main common-main.c:52:11 microsoft#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16 microsoft#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>

Add a failing test which demonstrates a regression in a18d66c ("diff.c: free "buf" in diff_words_flush()", 2022-03-04), the regression is discussed in detail in the subsequent commit. With it running `git show --word-diff --color-moved` with SANITIZE=address would emit: ==31191==ERROR: AddressSanitizer: attempting double-free on 0x617000021100 in thread T0: #0 0x49f0a2 in free (git+0x49f0a2) #1 0x9b0e4d in diff_words_flush diff.c:2153:3 microsoft#2 0x9aed5d in fn_out_consume diff.c:2354:3 microsoft#3 0xe092ab in consume_one xdiff-interface.c:43:9 microsoft#4 0xe072eb in xdiff_outf xdiff-interface.c:76:10 microsoft#5 0xec7014 in xdl_emit_diffrec xdiff/xutils.c:53:6 [...] 0x617000021100 is located 0 bytes inside of 768-byte region [0x617000021100,0x617000021400) freed by thread T0 here: #0 0x49f0a2 in free (git+0x49f0a2) [...(same stacktrace)...] previously allocated by thread T0 here: #0 0x49f603 in __interceptor_realloc (git+0x49f603) #1 0xde4da4 in xrealloc wrapper.c:126:8 microsoft#2 0x995dc5 in append_emitted_diff_symbol diff.c:794:2 microsoft#3 0x96c44a in emit_diff_symbol diff.c:1527:3 [...] This was not caught by the test suite because we test `diff --word-diff --color-moved` only so far. Therefore, add a test for `show`, too. Signed-off-by: Michael J Gruber <git@grubix.eu> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

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 microsoft#2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9 microsoft#3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2 microsoft#4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2 microsoft#5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2 microsoft#6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2 microsoft#7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11 microsoft#8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3 microsoft#9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4 microsoft#10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19 microsoft#11 0x562259 in main /home/avar/g/git/common-main.c:56:11 microsoft#12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16 microsoft#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>

The `linux-leaks` job has become a lot more aggressive, failing the following test cases: - t0001.52 extensions.objectFormat is not allowed with repo version 0 - t1302.3 gitdir selection on unsupported repo - t1302.4 gitdir not required mode - t1302.5 gitdir required mode - t1302.9 abort version=1 no-such-extension - t1302.12 abort version=0 noop-v1 The reason is that the `commondir` strbuf _is_ sometimes initialized _even if_ `discover_git_directory()` fails. The symptom: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x7f2e80ed8293 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x5603ff6ae674 in xrealloc wrapper.c:138 #2 0x5603ff661801 in strbuf_grow strbuf.c:101 #3 0x5603ff662417 in strbuf_add strbuf.c:300 #4 0x5603ff653e2b in strbuf_addstr strbuf.h:310 #5 0x5603ff65703b in setup_git_directory_gently_1 setup.c:1329 #6 0x5603ff65731b in discover_git_directory_reason setup.c:1388 #7 0x5603ff55b6c0 in discover_git_directory setup.h:79 #8 0x5603ff55b7ce in hook_path_early hook.c:35 #9 0x5603ff55b97d in find_hook hook.c:77 #10 0x5603ff55bcd3 in run_hooks_opt hook.c:189 #11 0x5603ff37ca05 in run_pre_command_hook git.c:457 #12 0x5603ff37ce0a in run_builtin git.c:532 #13 0x5603ff37d37e in handle_builtin git.c:798 #14 0x5603ff37d65a in run_argv git.c:867 #15 0x5603ff37dc8e in cmd_main git.c:1007 #16 0x5603ff48f4ee in main common-main.c:62 #17 0x7f2e80cabd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 The fix is easy: always release the `strbuf`s, even when the discovery of the Git directory failed. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Junio C Hamano <gitster@pobox.com>

An internal customer reported a segfault when running `git sparse-checkout set` with the `index.sparse` config enabled. I was unable to reproduce it locally, but with their help we debugged into the failing process and discovered the following stacktrace: ``` #0 0x00007ff6318fb7b0 in rehash (map=0x3dfb00d0440, newsize=1048576) at hashmap.c:125 #1 0x00007ff6318fbc66 in hashmap_add (map=0x3dfb00d0440, entry=0x3dfb5c58bc8) at hashmap.c:247 #2 0x00007ff631937a70 in hash_index_entry (istate=0x3dfb00d0400, ce=0x3dfb5c58bc8) at name-hash.c:122 #3 0x00007ff631938a2f in add_name_hash (istate=0x3dfb00d0400, ce=0x3dfb5c58bc8) at name-hash.c:638 #4 0x00007ff631a064de in set_index_entry (istate=0x3dfb00d0400, nr=8291, ce=0x3dfb5c58bc8) at sparse-index.c:255 #5 0x00007ff631a06692 in add_path_to_index (oid=0x5ff130, base=0x5ff580, path=0x3dfb4b725da "<redacted>", mode=33188, context=0x5ff570) at sparse-index.c:307 #6 0x00007ff631a3b48c in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41f60, base=0x5ff580, depth=2, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:46 #7 0x00007ff631a3b60b in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41e80, base=0x5ff580, depth=1, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:80 #8 0x00007ff631a3b60b in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41ac8, base=0x5ff580, depth=0, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:80 #9 0x00007ff631a06a95 in expand_index (istate=0x3dfb00d0100, pl=0x0) at sparse-index.c:422 #10 0x00007ff631a06cbd in ensure_full_index (istate=0x3dfb00d0100) at sparse-index.c:456 #11 0x00007ff631990d08 in index_name_stage_pos (istate=0x3dfb00d0100, name=0x3dfb0020080 "algorithm/levenshtein", namelen=21, stage=0, search_mode=EXPAND_SPARSE) at read-cache.c:556 #12 0x00007ff631990d6c in index_name_pos (istate=0x3dfb00d0100, name=0x3dfb0020080 "algorithm/levenshtein", namelen=21) at read-cache.c:566 #13 0x00007ff63180dbb5 in sanitize_paths (argc=185, argv=0x3dfb0030018, prefix=0x0, skip_checks=0) at builtin/sparse-checkout.c:756 #14 0x00007ff63180de50 in sparse_checkout_set (argc=185, argv=0x3dfb0030018, prefix=0x0) at builtin/sparse-checkout.c:860 #15 0x00007ff63180e6c5 in cmd_sparse_checkout (argc=186, argv=0x3dfb0030018, prefix=0x0) at builtin/sparse-checkout.c:1063 #16 0x00007ff6317234cb in run_builtin (p=0x7ff631ad9b38 <commands+2808>, argc=187, argv=0x3dfb0030018) at git.c:548 #17 0x00007ff6317239c0 in handle_builtin (argc=187, argv=0x3dfb0030018) at git.c:808 #18 0x00007ff631723c7d in run_argv (argcp=0x5ffdd0, argv=0x5ffd78) at git.c:877 #19 0x00007ff6317241d1 in cmd_main (argc=187, argv=0x3dfb0030018) at git.c:1017 #20 0x00007ff631838b60 in main (argc=190, argv=0x3dfb0030000) at common-main.c:64 ``` The very bottom of the stack being the `rehash()` method from `hashmap.c` as called within the `name-hash` API made me look at where these hashmaps were being used in the sparse index logic. These were being copied across indexes, which seems dangerous. Indeed, clearing these hashmaps and setting them as not initialized fixes the segfault. The second commit is a response to a test failure that happens in `t1092-sparse-checkout-compatibility.sh` where `git stash pop` starts to fail because the underlying `git checkout-index` process fails due to colliding files. Passing the `-f` flag appears to work, but it's unclear why this name-hash change causes that change in behavior.

kewillford approved these changes Jul 23, 2018

View reviewed changes

kewillford closed this Jul 24, 2018

kewillford reopened this Jul 24, 2018

kewillford closed this Jul 24, 2018

kewillford reopened this Jul 24, 2018

benpeart and others added 2 commits July 26, 2018 11:55

kewillford closed this Jul 31, 2018

kewillford reopened this Jul 31, 2018

benpeart merged commit f61ade4 into microsoft:gvfs-2.18.0 Aug 7, 2018

mjcheetham pushed a commit that referenced this pull request Jul 23, 2024

Post 2.46-rc0 batch #3

1c4a234

Signed-off-by: Junio C Hamano <gitster@pobox.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast track bug fix to merge-recursive #3

Fast track bug fix to merge-recursive #3

benpeart commented Jul 23, 2018 •

edited

Loading

dscho commented Jul 26, 2018

Fast track bug fix to merge-recursive #3

Fast track bug fix to merge-recursive #3

Conversation

benpeart commented Jul 23, 2018 • edited Loading

dscho commented Jul 26, 2018

benpeart commented Jul 23, 2018 •

edited

Loading