Fork Sync #1
Closed
Fork Sync #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Removal of GIT_TEST_GETTEXT_POISON continues. * ab/detox-gettext-tests: tests: remove most uses of test_i18ncmp tests: remove last uses of C_LOCALE_OUTPUT tests: remove most uses of C_LOCALE_OUTPUT tests: remove last uses of GIT_TEST_GETTEXT_POISON=false
Objects that lost references can be pruned away, even when they have notes attached to it (and these notes will become dangling, which in turn can be pruned with "git notes prune"). This has been clarified in the documentation. * mz/doc-notes-are-not-anchors: docs: clarify that refs/notes/ do not keep the attached objects alive
Optimization in "git blame" * rs/blame-optim: blame: remove unnecessary use of get_commit_info()
Docfix. * js/doc-proto-v2-response-end: doc: fix naming of response-end-pkt
The error codepath around the "--temp/--prefix" feature of "git checkout-index" has been improved. * mt/checkout-index-corner-cases: checkout-index: omit entries with no tempname from --temp output write_entry(): fix misuses of `path` in error messages
"git {diff,log} --{skip,rotate}-to=<path>" allows the user to
discard diff output for early paths or move them to the end of the
output.
* jc/diffcore-rotate:
diff: --{rotate,skip}-to=<path>
Docfix. * ma/doc-markup-fix: gitmailmap.txt: fix rendering of e-mail addresses git.txt: fix monospace rendering rev-list-options.txt: fix rendering of bonus paragraph
Doc update. * jc/maint-column-doc-typofix: Documentation: typofix --column description
Doc update. * cw/pack-config-doc: doc: mention bigFileThreshold for packing
"git difftool" learned "--skip-to=<path>" option to restart an interrupted session from an arbitrary path. * zh/difftool-skip-to: difftool.c: learn a new way start at specified file
Plug a minor memory leak. * ah/commit-graph-leakplug: commit-graph: avoid leaking topo_levels slab in write_commit_graph()
"git grep" has been tweaked to be limited to the sparse checkout paths. * mt/grep-sparse-checkout: grep: honor sparse-checkout on working tree searches
"git rebase --[no-]fork-point" gained a configuration variable rebase.forkPoint so that users do not have to keep specifying a non-default setting. * ah/rebase-no-fork-point-config: rebase: add a config option for --no-fork-point
The code to implement "git merge-base --independent" was poorly done and was kept from the very beginning of the feature. * ds/merge-base-independent: commit-reach: stale commits may prune generation further commit-reach: use heuristic in remove_redundant() commit-reach: move compare_commits_by_gen commit-reach: use one walk in remove_redundant() commit-reach: reduce requirements for remove_redundant()
Various fixes on "git add --chmod". * mt/add-chmod-fixes: add: propagate --chmod errors to exit status add: mark --chmod error string for translation add --chmod: don't update index when --dry-run is used
The "git maintenance register" command had trouble registering bare repositories, which had been corrected. * es/maintenance-of-bare-repositories: maintenance: fix incorrect `maintenance.repo` path with bare repository
Doc update. * ug/doc-commit-approxidate: doc: mention approxidates for git-commit --date
Messages update. * js/params-vs-args: replace "parameters" by "arguments" in error messages
A handful of multi-word configuration variable names in documentation that are spelled in all lowercase have been corrected to use the more canonical camelCase. * dl/doc-config-camelcase: index-format doc: camelCase core.excludesFile blame-options.txt: camelcase blame.blankBoundary i18n.txt: camel case and monospace "i18n.commitEncoding"
Mergetools update. * sh/mergetools-vimdiff1: mergetools/vimdiff: add vimdiff1 merge tool variant
"git push $there --delete ''" should have been diagnosed as an error, but instead turned into a matching push, which has been corrected. * jc/push-delete-nothing: push: do not turn --delete '' into a matching push
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Generate po/git.pot from v2.31.0-rc0 for git v2.31.0 l10n round 1. Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
The gitattributes documentation mentions that either the clean cmd or the smudge cmd can be left unspecified in a filter definition. However, when the filter is marked as 'required', the absence of any one of these two should be treated as an error. Git already fails under these circumstances, but not always in a pleasant way: omitting a clean cmd in a required filter triggers an assertion error which leaves the user with a quite verbose message: git: convert.c:1459: convert_to_git_filter_fd: Assertion "ca.drv->clean || ca.drv->process" failed. This assertion is not really necessary, as the apply_filter() call below it already performs the same check. And when this condition is not met, the function returns 0, making the caller die() with a much nicer message. (Also note that die()-ing here is the right behavior as `would_convert_to_git_filter_fd() == true` is a precondition to use convert_to_git_filter_fd(), and the former is only true when the filter is required.) So remove the assertion and add two regression tests to make sure that git fails nicely when either the smudge or clean command is missing on a required filter. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
On some platforms, open() reportedly returns EINTR when opening regular files and we receive a signal (usually SIGALRM from our progress meter). This shouldn't happen, as open() should be a restartable syscall, and we specify SA_RESTART when setting up the alarm handler. So it may actually be a kernel or libc bug for this to happen. But it has been reported on at least one version of Linux (on a network filesystem): https://lore.kernel.org/git/c8061cce-71e4-17bd-a56a-a5fed93804da@neanderfunk.de/ as well as on macOS starting with Big Sur even on a regular filesystem. We can work around it by retrying open() calls that get EINTR, just as we do for read(), etc. Since we don't ever _want_ to interrupt an open() call, we can get away with just redefining open, rather than insisting all callsites use xopen(). We actually do have an xopen() wrapper already (and it even does this retry, though there's no indication of it being an observed problem back then; it seems simply to have been lifted from xread(), etc). But it is used hardly anywhere, and isn't suitable for general use because it will die() on error. In theory we could combine the two, but it's awkward to do so because of the variable-args interface of open(). This patch adds a Makefile knob for enabling the workaround. It's not enabled by default for any platforms in config.mak.uname yet, as we don't have enough data to decide how common this is (I have not been able to reproduce on either Linux or Big Sur myself). It may be worth enabling preemptively anyway, since the cost is pretty low (if we don't see an EINTR, it's just an extra conditional). However, note that we must not enable this on Windows. It doesn't do anything there, and the macro overrides the existing mingw_open() redirection. I've added a preemptive #undef here in the mingw header (which is processed first) to just quietly disable it (we could also make it an #error, but there is little point in being so aggressive). Reported-by: Aleksey Kliger <alklig@microsoft.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The perf suite gets confused when test_perf_default_repo is pointed at a worktree (which includes when it is run from within a worktree at all, since the default is to use the current repository). Here's an example: $ git worktree add ~/foo Preparing worktree (new branch 'foo') HEAD is now at 328c109 The eighth batch $ cd ~/foo $ make [...build output...] $ cd t/perf $ ./p0000-perf-lib-sanity.sh -v -i [...] perf 1 - test_perf_default_repo works: running: foo=$(git rev-parse HEAD) && test_export foo fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' The problem is that we didn't copy all of the necessary files from the source repository (in this case we got HEAD, but we have no refs!). We discover the git-dir with "rev-parse --git-dir", but this points to the worktree's partial repository in .../.git/worktrees/foo. That partial repository has a "commondir" file which points to the main repository, where the actual refs are stored, but we don't copy it. This is the correct thing to do, though! If we did copy it, then our scratch test repo would be pointing back to the original main repo, and any ref updates we made in the tests would impact that original repo. Instead, we need to either: 1. Make a scratch copy of the original main repo (in addition to the worktree repo), and point the scratch worktree repo's commondir at it. This preserves the original relationship, but it's doubtful any script really cares (if they are testing worktree performance, they'd probably make their own worktrees). And it's trickier to get right. 2. Collapse the main and worktree repos into a single scratch repo. This can be done by copying everything from both, preferring any files from the worktree repo. This patch does the second one. With this applied, the example above results in p0000 running successfully. Reported-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When running the perf suite, we copy files from an existing $GIT_DIR to
a scratch repository to give us a realistic setup on which to operate.
Since the perf scripts themselves may modify the scratch repository, we
want to make sure we've scrubbed any references back to the original.
One existing example is that we avoid copying the file "commondir" at
the top-level of the repository. In a worktree git-dir (e.g.,
.git/worktrees/foo), that file contains the path to the parent
repository; copying it could mean ref updates in the scratch repository
affect the original.
But there are other files we should cover, too:
- "gitdir" in a worktree git-dir contains the path to the actual .git
file in the working tree. We _shouldn't_ end up looking at it at
all, since the lack of a "commondir" file means Git won't consider
this to be a worktree git-dir. But it's best to err on the safe
side.
- in a parent repository that contains worktrees, the
"$GIT_DIR/worktrees" directory will contain the git dirs for the
individual worktrees. Which will themselves contain commondir and
gitdir files that may reference the original repository. We should
likewise remove them.
Note that this does mean that the perf suite's scratch repositories
will never have any worktrees. That's OK; we don't have any perf tests
that are influenced by their presence. If we add any, they'd
probably want to create the worktrees themselves anyway.
This patch adds both paths to the set of omissions in
test_perf_copy_repo_contents(). Note that we won't get confused here by
matching arbitrary names like refs/heads/commondir. This list is always
matching top-level entries in $GIT_DIR (we rely on "cp -R" to do the
actual recursion).
Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When opening a reverse index, load_revindex_from_disk() jumps to the 'cleanup' label in case something goes wrong: the reverse index had the wrong size, an unrecognized version, or similar. It also jumps to this label when the reverse index couldn't be opened in the first place, which will cause an error with the unguarded close() call in the label. Guard this call with "if (fd >= 0)" to make sure that we have a valid file descriptor to close before attempting to close it. Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
A previous commit noted that it is very common for people to move files across directories while keeping their filename the same. The last few commits took advantage of this and showed that we can accelerate rename detection significantly using basenames; since files with the same basename serve as likely rename candidates, we can check those first and remove them from the rename candidate pool if they are sufficiently similar. Unfortunately, the previous optimization was limited by the fact that the remaining basenames after exact rename detection are not always unique. Many repositories have hundreds of build files with the same name (e.g. Makefile, .gitignore, build.gradle, etc.), and may even have hundreds of source files with the same name. (For example, the linux kernel has 100 setup.c, 87 irq.c, and 112 core.c files. A repository at $DAYJOB has a lot of ObjectFactory.java and Plugin.java files). For these files with non-unique basenames, we are faced with the task of attempting to determine or guess which directory they may have been relocated to. Such a task is precisely the job of directory rename detection. However, there are two catches: (1) the directory rename detection code has traditionally been part of the merge machinery rather than diffcore-rename.c, and (2) directory rename detection currently runs after regular rename detection is complete. The 1st catch is just an implementation issue that can be overcome by some code shuffling. The 2nd requires us to add a further approximation: we only have access to exact renames at this point, so we need to do directory rename detection based on just exact renames. In some cases we won't have exact renames, in which case this extra optimization won't apply. We also choose to not apply the optimization unless we know that the underlying directory was removed, which will require extra data to be passed in to diffcore_rename_extended(). Also, even if we get a prediction about which directory a file may have relocated to, we will still need to check to see if there is a file in the predicted directory, and then compare the two files to see if they meet the higher min_basename_score threshold required for marking the two files as renames. This commit introduces an idx_possible_rename() function which will do this directory rename detection for us and give us the index within rename_dst of the resulting filename. For now, this function is hardcoded to return -1 (not found) and just hooks up how its results would be used once we have a more complete implementation in place. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a new struct dir_rename_info with various values we need inside our idx_possible_rename() function introduced in the previous commit. Add a basic implementation for this function showing how we plan to use the variables, but which will just return early with a value of -1 (not found) when those variables are not set up. Future commits will do the work necessary to set up those other variables so that idx_possible_rename() does not always return -1. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 25, 2021
setup_unpack_trees_porcelain() populates various fields on
unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to
avoid leaking them. Specifically, we used to leak
unpack_tree_opts.msgs_to_free.
We have to do this in leave_reset_head because there are multiple
scenarios where unpack_tree_opts has already been configured, followed
by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'
prior to having initialised unpack_tree_opts via memset(..., 0, ...).
Therefore we also move unpack_tree_opts initialisation to the start of
reset_head(), and convert it to use brace initialisation - which
guarantees that we can never clear an uninitialised unpack_tree_opts.
clear_unpack_tree_opts() is always safe to call as long as
unpack_tree_opts is at least zero-initialised, i.e. it does not depend
on a previous call to setup_unpack_trees_porcelain().
LSAN output from t0021:
Direct leak of 192 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9f7861 in strvec_push_nodup strvec.c:19:2
#3 0x9f7861 in strvec_pushf strvec.c:39:2
#4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
#5 0x97e011 in reset_head reset.c:53:2
#6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
#7 0x4ce83e in run_builtin git.c:475:11
git#8 0x4ccafe in handle_builtin git.c:729:3
git#9 0x4cb01c in run_argv git.c:818:4
git#10 0x4cb01c in cmd_main git.c:949:19
git#11 0x6b3f3d in main common-main.c:52:11
git#12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 147 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 134 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 130 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
github-actions bot
pushed a commit
that referenced
this pull request
Jul 26, 2021
During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one and expires it. This behavior has been with us since this command was implemented in 4264dc1 ("git reflog expire", 2006-12-19). Change this to stop calling lock_ref_oid_basic() with the OID we saw when we looped over the logs, instead have it pass the OID it managed to lock. This mostly mitigates a race condition where e.g. "git gc" will fail in a concurrently updated repository because the branch moved since "git reflog expire --all" was started. I.e. with: error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B> This behavior of passing in an "oid" was needed for an edge-case that I've untangled in this and preceding commits though, namely that we needed this OID because we'd: 1. Lookup the reflog name/OID via dwim_log() 2. With that OID, lock the reflog 3. Later in builtin/reflog.c we use the OID we looked as input to lookup_commit_reference_gently(), assured that it's equal to the OID we got from dwim_log(). We can be sure that this change is safe to make because between dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other logic relevant to the OID or expiry run in the cmd_reflog_expire() caller. We can thus treat that code as a black box, before and after this change it would get an OID that's been locked, the only difference is that now we mostly won't be failing to get the lock due to the TOCTOU race[0]. That failure was purely an implementation detail in how the "current OID" was looked up, it was divorced from the locking mechanism. What do we mean with "mostly"? It mostly mitigates it because we'll still run into cases where the ref is locked and being updated as we want to expire it, and other git processes wanting to update the refs will in turn race with us as we expire the reflog. That remaining race can in turn be mitigated with the core.filesRefLockTimeout setting, see 4ff0f01 ("refs: retry acquiring reference locks for 100ms", 2017-08-21). In practice if that value is high enough we'll probably never have ref updates or reflog expiry failing, since the clients involved will retry for far longer than the time any of those operations could take. See [1] for an initial report of how this impacted "git gc" and a large discussion about this change in early 2019. In particular patch looked good to Michael Haggerty, see his[2]. That message seems to not have made it to the ML archive, its content is quoted in full in my [3]. I'm leaving behind now-unused code the refs API etc. that takes the now-NULL "oid" argument, and other code that can be simplified now that we never have on OID in that context, that'll be cleaned up in subsequent commits, but for now let's narrowly focus on fixing the "git gc" issue. As the modified assert() shows we always pass a NULL oid to reflog_expire() now. Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals: ( rm -rf /tmp/git && git init /tmp/git && while true do head -c 10 /dev/urandom | hexdump >/tmp/git/out && git -C /tmp/git add out && git -C /tmp/git commit -m"out" done ) ( rm -rf /tmp/git-clone && git clone file:///tmp/git /tmp/git-clone && while git -C /tmp/git-clone pull do date done ) ( while git -C /tmp/git-clone reflog expire --all do date done ) Before this change the "reflog expire" would fail really quickly with the "but expected" error noted above. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). As noted above that can in turn be mitigated with higher values of core.filesRefLockTimeout than the 100ms default. As noted in the commentary added in the preceding commit there's also the case of branches being racily deleted, that can be tested by adding this to the above: ( while git -C /tmp/git-clone branch topic master && git -C /tmp/git-clone branch -D topic do date done ) With core.filesRefLockTimeout set to 10 seconds (it can probably be a lot lower) I managed to run all four of these concurrently for about an hour, and accumulated ~125k commits, auto-gc's and all, and didn't have a single failure. The loops visibly stall while waiting for the lock, but that's expected and desired behavior. 0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use 1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/ 2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu 3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
origin starts off pointing to somewhere within line, which is owned by
the caller. Later we might allocate a new string using xmemdupz() or
xstrfmt(). To avoid leaking these new strings, we introduce a to_free
pointer - which allows us to safely free the newly allocated string when
we're done (we cannot just free origin directly as it might still be
pointing to line).
LSAN output from t0090:
Direct leak of 8 byte(s) in 1 object(s) allocated from:
#0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0xa71f49 in do_xmalloc wrapper.c:41:8
#2 0xa720b0 in do_xmallocz wrapper.c:75:8
#3 0xa720b0 in xmallocz wrapper.c:83:9
#4 0xa720b0 in xmemdupz wrapper.c:99:16
#5 0x8092ba in handle_line fmt-merge-msg.c:187:23
#6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7
#7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2
git#8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3
git#9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16
git#10 0x4ce83e in run_builtin git.c:475:11
git#11 0x4ccafe in handle_builtin git.c:729:3
git#12 0x4cb01c in run_argv git.c:818:4
git#13 0x4cb01c in cmd_main git.c:949:19
git#14 0x6b3fad in main common-main.c:52:11
git#15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
realpath is only populated if we execute the git_work_tree_initialized
block. However that block also causes us to return early, meaning we
never actually release the strbuf in the case where we populated it.
Therefore we move all strbuf related code into the block to guarantee
that we can't leak it.
LSAN output from t0095:
Direct leak of 129 byte(s) in 1 object(s) allocated from:
#0 0x49a9b9 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0x78f585 in xrealloc wrapper.c:126:8
#2 0x713ff4 in strbuf_grow strbuf.c:98:2
#3 0x713ff4 in strbuf_getcwd strbuf.c:597:3
#4 0x4f0c18 in strbuf_realpath_1 abspath.c:99:7
#5 0x5ae4a4 in set_git_work_tree environment.c:259:3
#6 0x6fdd8a in setup_discovered_git_dir setup.c:931:2
#7 0x6fdd8a in setup_git_directory_gently setup.c:1235:12
git#8 0x4cb50d in get_bloom_filter_for_commit t/helper/test-bloom.c:41:2
git#9 0x4cb50d in cmd__bloom t/helper/test-bloom.c:95:3
git#10 0x4caa1f in cmd_main t/helper/test-tool.c:124:11
git#11 0x4caded in main common-main.c:52:11
git#12 0x7f0869f02349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 129 byte(s) leaked in 1 allocation(s).
It looks like this leak has existed since realpath was first added to
set_git_work_tree() in:
3d7747e (real_path: remove unsafe API, 2020-03-10)
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
relative_url() populates sb. In the normal return path, its buffer is
detached using strbuf_detach(). However the early return path does
nothing with sb, which means that sb's memory is leaked - therefore
we add a release to avoid this leak.
The reset is also only necessary for the normal return path, hence we
move it down to after the early-return to avoid unnecessary work.
LSAN output from t0060:
Direct leak of 121 byte(s) in 1 object(s) allocated from:
#0 0x7f31246f28b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
#1 0x98d7d6 in xrealloc wrapper.c:126
#2 0x909a60 in strbuf_grow strbuf.c:98
#3 0x90bf00 in strbuf_vaddf strbuf.c:401
#4 0x90c321 in strbuf_addf strbuf.c:335
#5 0x5cb78d in relative_url builtin/submodule--helper.c:182
#6 0x5cbe46 in resolve_relative_url_test builtin/submodule--helper.c:248
#7 0x410dcd in run_builtin git.c:475
git#8 0x410dcd in handle_builtin git.c:729
git#9 0x414087 in run_argv git.c:818
git#10 0x414087 in cmd_main git.c:949
git#11 0x40e9ec in main common-main.c:52
git#12 0x7f3123c41349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 121 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
cmd_for_each_repo() copies argv into args (a strvec), which is later
passed into run_command_on_repo(), which in turn copies that strvec onto
the end of child.args. The initial copy is unnecessary (we never modify
args). We therefore choose to just pass argv directly into
run_command_on_repo(), which lets us avoid the copy and fixes the leak.
LSAN output from t0068:
Direct leak of 192 byte(s) in 1 object(s) allocated from:
#0 0x7f63bd4ab8b0 in realloc (/usr/lib64/libasan.so.4+0xdc8b0)
#1 0x98d7e6 in xrealloc wrapper.c:126
#2 0x916914 in strvec_push_nodup strvec.c:19
#3 0x916a6e in strvec_push strvec.c:26
#4 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
#5 0x410dcd in run_builtin git.c:475
#6 0x410dcd in handle_builtin git.c:729
#7 0x414087 in run_argv git.c:818
git#8 0x414087 in cmd_main git.c:949
git#9 0x40e9ec in main common-main.c:52
git#10 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 22 byte(s) in 2 object(s) allocated from:
#0 0x7f63bd445e30 in __interceptor_strdup (/usr/lib64/libasan.so.4+0x76e30)
#1 0x98d698 in xstrdup wrapper.c:29
#2 0x916a63 in strvec_push strvec.c:26
#3 0x4be4eb in cmd_for_each_repo builtin/for-each-repo.c:49
#4 0x410dcd in run_builtin git.c:475
#5 0x410dcd in handle_builtin git.c:729
#6 0x414087 in run_argv git.c:818
#7 0x414087 in cmd_main git.c:949
git#8 0x40e9ec in main common-main.c:52
git#9 0x7f63bc9fa349 in __libc_start_main (/lib64/libc.so.6+0x24349)
See also discussion about the original implementation below - this code
appears to have evolved from a callback explaining the double-strvec-copy
pattern, but there's no strong reason to keep that now:
https://lore.kernel.org/git/68bbeca5-314b-08ee-ef36-040e3f3814e9@gmail.com/
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
old_dir/new_dir are free()'d at the end of update_dir_rename_counts, however if we return early we'll never free those strings. Therefore we should move all new allocations after the possible early return, avoiding a leak. This seems like a fairly recent leak, that started happening since the early-return was added in: 1ad69eb (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27) LSAN output from t0022: Direct leak of 7 byte(s) in 1 object(s) allocated from: #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0xa71e48 in xstrdup wrapper.c:29:14 #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12 #3 0x7db6ae in find_renames diffcore-rename.c:1062:3 #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18 #5 0x7b4cfc in diffcore_std diff.c:6705:4 #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2 #7 0x856574 in log_tree_diff log-tree.c:955:3 git#8 0x856574 in log_tree_commit log-tree.c:986:10 git#9 0x9a9c67 in print_commit_summary sequencer.c:1329:7 git#10 0x52e623 in cmd_commit builtin/commit.c:1862:3 git#11 0x4ce83e in run_builtin git.c:475:11 git#12 0x4ccafe in handle_builtin git.c:729:3 git#13 0x4cb01c in run_argv git.c:818:4 git#14 0x4cb01c in cmd_main git.c:949:19 git#15 0x6b3f3d in main common-main.c:52:11 git#16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349) Direct leak of 7 byte(s) in 1 object(s) allocated from: #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 #1 0xa71e48 in xstrdup wrapper.c:29:14 #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12 #3 0x7db6ae in find_renames diffcore-rename.c:1062:3 #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18 #5 0x7b4cfc in diffcore_std diff.c:6705:4 #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2 #7 0x856574 in log_tree_diff log-tree.c:955:3 git#8 0x856574 in log_tree_commit log-tree.c:986:10 git#9 0x9a9c67 in print_commit_summary sequencer.c:1329:7 git#10 0x52e623 in cmd_commit builtin/commit.c:1862:3 git#11 0x4ce83e in run_builtin git.c:475:11 git#12 0x4ccafe in handle_builtin git.c:729:3 git#13 0x4cb01c in run_argv git.c:818:4 git#14 0x4cb01c in cmd_main git.c:949:19 git#15 0x6b3f3d in main common-main.c:52:11 git#16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s). Signed-off-by: Andrzej Hunt <andrzej@ahunt.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
u.head is populated using resolve_refdup(), which returns a newly
allocated string - hence we also need to free() it.
Found while running t0041 with LSAN:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0xa8be98 in xstrdup wrapper.c:29:14
#2 0x9481db in head_atom_parser ref-filter.c:549:17
#3 0x9408c7 in parse_ref_filter_atom ref-filter.c:703:30
#4 0x9400e3 in verify_ref_format ref-filter.c:974:8
#5 0x4f9e8b in print_ref_list builtin/branch.c:439:6
#6 0x4f9e8b in cmd_branch builtin/branch.c:757:3
#7 0x4ce83e in run_builtin git.c:475:11
git#8 0x4ccafe in handle_builtin git.c:729:3
git#9 0x4cb01c in run_argv git.c:818:4
git#10 0x4cb01c in cmd_main git.c:949:19
git#11 0x6bdc2d in main common-main.c:52:11
git#12 0x7f96edf86349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
repo_diff_setup() calls through to diff.c's static prep_parse_options(),
which in turn allocates a new array into diff_opts.parseopts.
diff_setup_done() is responsible for freeing that array, and has the
benefit of verifying diff_opts too - hence we add a call to
diff_setup_done() to avoid leaking parseopts.
Output from the leak as found while running t0090 with LSAN:
Direct leak of 7120 byte(s) in 1 object(s) allocated from:
#0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0xa8bf89 in do_xmalloc wrapper.c:41:8
#2 0x7a7bae in prep_parse_options diff.c:5636:2
#3 0x7a7bae in repo_diff_setup diff.c:4611:2
#4 0x93716c in repo_index_has_changes read-cache.c:2518:3
#5 0x872233 in unclean merge-ort-wrappers.c:12:14
#6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
#7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
git#8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
git#9 0x4ce83e in run_builtin git.c:475:11
git#10 0x4ccafe in handle_builtin git.c:729:3
git#11 0x4cb01c in run_argv git.c:818:4
git#12 0x4cb01c in cmd_main git.c:949:19
git#13 0x6bdc2d in main common-main.c:52:11
git#14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
apply_multi_file_filter and async_query_available_blobs both query
subprocess output using subprocess_read_status, which writes data into
the identically named filter_status strbuf. We add a strbuf_release to
avoid leaking their contents.
Leak output seen when running t0021 with LSAN:
Direct leak of 24 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa8c2b5 in xrealloc wrapper.c:126:8
#2 0x9ff99d in strbuf_grow strbuf.c:98:2
#3 0x9ff99d in strbuf_addbuf strbuf.c:304:2
#4 0xa101d6 in subprocess_read_status sub-process.c:45:5
#5 0x77793c in apply_multi_file_filter convert.c:886:8
#6 0x77793c in apply_filter convert.c:1042:10
#7 0x77a0b5 in convert_to_git_filter_fd convert.c:1492:7
git#8 0x8b48cd in index_stream_convert_blob object-file.c:2156:2
git#9 0x8b48cd in index_fd object-file.c:2248:9
git#10 0x597411 in hash_fd builtin/hash-object.c:43:9
git#11 0x596be1 in hash_object builtin/hash-object.c:59:2
git#12 0x596be1 in cmd_hash_object builtin/hash-object.c:153:3
git#13 0x4ce83e in run_builtin git.c:475:11
git#14 0x4ccafe in handle_builtin git.c:729:3
git#15 0x4cb01c in run_argv git.c:818:4
git#16 0x4cb01c in cmd_main git.c:949:19
git#17 0x6bdc2d in main common-main.c:52:11
git#18 0x7f42acf79349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).
Direct leak of 120 byte(s) in 5 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa8c295 in xrealloc wrapper.c:126:8
#2 0x9ff97d in strbuf_grow strbuf.c:98:2
#3 0x9ff97d in strbuf_addbuf strbuf.c:304:2
#4 0xa101b6 in subprocess_read_status sub-process.c:45:5
#5 0x775c73 in async_query_available_blobs convert.c:960:8
#6 0x80029d in finish_delayed_checkout entry.c:183:9
#7 0xa65d1e in check_updates unpack-trees.c:493:10
git#8 0xa5f469 in unpack_trees unpack-trees.c:1747:8
git#9 0x525971 in checkout builtin/clone.c:815:6
git#10 0x525971 in cmd_clone builtin/clone.c:1409:8
git#11 0x4ce83e in run_builtin git.c:475:11
git#12 0x4ccafe in handle_builtin git.c:729:3
git#13 0x4cb01c in run_argv git.c:818:4
git#14 0x4cb01c in cmd_main git.c:949:19
git#15 0x6bdc2d in main common-main.c:52:11
git#16 0x7fa253fce349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 120 byte(s) leaked in 5 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
These leaks all happen at the end of cmd_mv, hence don't matter in any
way. But we still fix the easy ones and squash the rest to get us closer
to being able to run tests without leaks.
LSAN output from t0050:
Direct leak of 384 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa8c015 in xrealloc wrapper.c:126:8
#2 0xa0a7e1 in add_entry string-list.c:44:2
#3 0xa0a7e1 in string_list_insert string-list.c:58:14
#4 0x5dac03 in cmd_mv builtin/mv.c:248:4
#5 0x4ce83e in run_builtin git.c:475:11
#6 0x4ccafe in handle_builtin git.c:729:3
#7 0x4cb01c in run_argv git.c:818:4
git#8 0x4cb01c in cmd_main git.c:949:19
git#9 0x6bd9ad in main common-main.c:52:11
git#10 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0xa8bd09 in do_xmalloc wrapper.c:41:8
#2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
#3 0x5da575 in cmd_mv builtin/mv.c:158:14
#4 0x4ce83e in run_builtin git.c:475:11
#5 0x4ccafe in handle_builtin git.c:729:3
#6 0x4cb01c in run_argv git.c:818:4
#7 0x4cb01c in cmd_main git.c:949:19
git#8 0x6bd9ad in main common-main.c:52:11
git#9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0xa8bd09 in do_xmalloc wrapper.c:41:8
#2 0x5dbc34 in internal_prefix_pathspec builtin/mv.c:32:2
#3 0x5da4e4 in cmd_mv builtin/mv.c:148:11
#4 0x4ce83e in run_builtin git.c:475:11
#5 0x4ccafe in handle_builtin git.c:729:3
#6 0x4cb01c in run_argv git.c:818:4
#7 0x4cb01c in cmd_main git.c:949:19
git#8 0x6bd9ad in main common-main.c:52:11
git#9 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 8 byte(s) in 1 object(s) allocated from:
#0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
#1 0xa8c119 in xcalloc wrapper.c:140:8
#2 0x5da585 in cmd_mv builtin/mv.c:159:22
#3 0x4ce83e in run_builtin git.c:475:11
#4 0x4ccafe in handle_builtin git.c:729:3
#5 0x4cb01c in run_argv git.c:818:4
#6 0x4cb01c in cmd_main git.c:949:19
#7 0x6bd9ad in main common-main.c:52:11
git#8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 4 byte(s) in 1 object(s) allocated from:
#0 0x49a9a2 in calloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
#1 0xa8c119 in xcalloc wrapper.c:140:8
#2 0x5da4f8 in cmd_mv builtin/mv.c:149:10
#3 0x4ce83e in run_builtin git.c:475:11
#4 0x4ccafe in handle_builtin git.c:729:3
#5 0x4cb01c in run_argv git.c:818:4
#6 0x4cb01c in cmd_main git.c:949:19
#7 0x6bd9ad in main common-main.c:52:11
git#8 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 65 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa8c015 in xrealloc wrapper.c:126:8
#2 0xa00226 in strbuf_grow strbuf.c:98:2
#3 0xa00226 in strbuf_vaddf strbuf.c:394:3
#4 0xa065c7 in xstrvfmt strbuf.c:981:2
#5 0xa065c7 in xstrfmt strbuf.c:991:8
#6 0x9e7ce7 in prefix_path_gently setup.c:115:15
#7 0x9e7fa6 in prefix_path setup.c:128:12
git#8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
git#9 0x5da575 in cmd_mv builtin/mv.c:158:14
git#10 0x4ce83e in run_builtin git.c:475:11
git#11 0x4ccafe in handle_builtin git.c:729:3
git#12 0x4cb01c in run_argv git.c:818:4
git#13 0x4cb01c in cmd_main git.c:949:19
git#14 0x6bd9ad in main common-main.c:52:11
git#15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 65 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa8c015 in xrealloc wrapper.c:126:8
#2 0xa00226 in strbuf_grow strbuf.c:98:2
#3 0xa00226 in strbuf_vaddf strbuf.c:394:3
#4 0xa065c7 in xstrvfmt strbuf.c:981:2
#5 0xa065c7 in xstrfmt strbuf.c:991:8
#6 0x9e7ce7 in prefix_path_gently setup.c:115:15
#7 0x9e7fa6 in prefix_path setup.c:128:12
git#8 0x5dbdbf in internal_prefix_pathspec builtin/mv.c:55:23
git#9 0x5da4e4 in cmd_mv builtin/mv.c:148:11
git#10 0x4ce83e in run_builtin git.c:475:11
git#11 0x4ccafe in handle_builtin git.c:729:3
git#12 0x4cb01c in run_argv git.c:818:4
git#13 0x4cb01c in cmd_main git.c:949:19
git#14 0x6bd9ad in main common-main.c:52:11
git#15 0x7fbfeffc4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 558 byte(s) leaked in 7 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
merge_name() calls dwim_ref(), which allocates a new string into
found_ref. Therefore add a free() to avoid leaking found_ref.
LSAN output from t0021:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0xa8beb8 in xstrdup wrapper.c:29:14
#2 0x954054 in expand_ref refs.c:671:12
#3 0x953cb6 in repo_dwim_ref refs.c:644:22
#4 0x5d3759 in dwim_ref refs.h:162:9
#5 0x5d3759 in merge_name builtin/merge.c:517:6
#6 0x5d3759 in collect_parents builtin/merge.c:1214:5
#7 0x5cf60d in cmd_merge builtin/merge.c:1458:16
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6bdbfd in main common-main.c:52:11
git#13 0x7f0430502349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
- cmd_rebase populates rebase_options.strategy with newly allocated
strings, hence we need to free those strings at the end of cmd_rebase
to avoid a leak.
- In some cases: get_replay_opts() is called, which prepares replay_opts
using data from rebase_options. We used to simply copy the pointer
from rebase_options.strategy, however that would now result in a
double-free because sequencer_remove_state() is eventually used to
free replay_opts.strategy. To avoid this we xstrdup() strategy when
adding it to replay_opts.
The original leak happens because we always populate
rebase_options.strategy, but we don't always enter the path that calls
get_replay_opts() and later sequencer_remove_state() - in other words
we'd always allocate a new string into rebase_options.strategy but
only sometimes did we free it. We now make sure that rebase_options
and replay_opts both own their own copies of strategy, and each copy
is free'd independently.
This was first seen when running t0021 with LSAN, but t2012 helped catch
the fact that we can't just free(options.strategy) at the end of
cmd_rebase (as that can cause a double-free). LSAN output from t0021:
LSAN output from t0021:
Direct leak of 4 byte(s) in 1 object(s) allocated from:
#0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0xa71eb8 in xstrdup wrapper.c:29:14
#2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22
#3 0x4ce83e in run_builtin git.c:475:11
#4 0x4ccafe in handle_builtin git.c:729:3
#5 0x4cb01c in run_argv git.c:818:4
#6 0x4cb01c in cmd_main git.c:949:19
#7 0x6b3fad in main common-main.c:52:11
git#8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Jul 31, 2021
setup_unpack_trees_porcelain() populates various fields on
unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to
avoid leaking them. Specifically, we used to leak
unpack_tree_opts.msgs_to_free.
We have to do this in leave_reset_head because there are multiple
scenarios where unpack_tree_opts has already been configured, followed
by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head'
prior to having initialised unpack_tree_opts via memset(..., 0, ...).
Therefore we also move unpack_tree_opts initialisation to the start of
reset_head(), and convert it to use brace initialisation - which
guarantees that we can never clear an uninitialised unpack_tree_opts.
clear_unpack_tree_opts() is always safe to call as long as
unpack_tree_opts is at least zero-initialised, i.e. it does not depend
on a previous call to setup_unpack_trees_porcelain().
LSAN output from t0021:
Direct leak of 192 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9f7861 in strvec_push_nodup strvec.c:19:2
#3 0x9f7861 in strvec_pushf strvec.c:39:2
#4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
#5 0x97e011 in reset_head reset.c:53:2
#6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
#7 0x4ce83e in run_builtin git.c:475:11
git#8 0x4ccafe in handle_builtin git.c:729:3
git#9 0x4cb01c in run_argv git.c:818:4
git#10 0x4cb01c in cmd_main git.c:949:19
git#11 0x6b3f3d in main common-main.c:52:11
git#12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 147 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 134 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 130 byte(s) in 1 object(s) allocated from:
#0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
#1 0xa721e5 in xrealloc wrapper.c:126:8
#2 0x9e8d54 in strbuf_grow strbuf.c:98:2
#3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3
#4 0x9f7774 in strvec_pushf strvec.c:36:2
#5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3
#6 0x97e011 in reset_head reset.c:53:2
#7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9
git#8 0x4ce83e in run_builtin git.c:475:11
git#9 0x4ccafe in handle_builtin git.c:729:3
git#10 0x4cb01c in run_argv git.c:818:4
git#11 0x4cb01c in cmd_main git.c:949:19
git#12 0x6b3f3d in main common-main.c:52:11
git#13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
github-actions bot
pushed a commit
that referenced
this pull request
Aug 5, 2021
During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one and expires it. This behavior has been with us since this command was implemented in 4264dc1 ("git reflog expire", 2006-12-19). Change this to stop calling lock_ref_oid_basic() with the OID we saw when we looped over the logs, instead have it pass the OID it managed to lock. This mostly mitigates a race condition where e.g. "git gc" will fail in a concurrently updated repository because the branch moved since "git reflog expire --all" was started. I.e. with: error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B> This behavior of passing in an "oid" was needed for an edge-case that I've untangled in this and preceding commits though, namely that we needed this OID because we'd: 1. Lookup the reflog name/OID via dwim_log() 2. With that OID, lock the reflog 3. Later in builtin/reflog.c we use the OID we looked as input to lookup_commit_reference_gently(), assured that it's equal to the OID we got from dwim_log(). We can be sure that this change is safe to make because between dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other logic relevant to the OID or expiry run in the cmd_reflog_expire() caller. We can thus treat that code as a black box, before and after this change it would get an OID that's been locked, the only difference is that now we mostly won't be failing to get the lock due to the TOCTOU race[0]. That failure was purely an implementation detail in how the "current OID" was looked up, it was divorced from the locking mechanism. What do we mean with "mostly"? It mostly mitigates it because we'll still run into cases where the ref is locked and being updated as we want to expire it, and other git processes wanting to update the refs will in turn race with us as we expire the reflog. That remaining race can in turn be mitigated with the core.filesRefLockTimeout setting, see 4ff0f01 ("refs: retry acquiring reference locks for 100ms", 2017-08-21). In practice if that value is high enough we'll probably never have ref updates or reflog expiry failing, since the clients involved will retry for far longer than the time any of those operations could take. See [1] for an initial report of how this impacted "git gc" and a large discussion about this change in early 2019. In particular patch looked good to Michael Haggerty, see his[2]. That message seems to not have made it to the ML archive, its content is quoted in full in my [3]. I'm leaving behind now-unused code the refs API etc. that takes the now-NULL "unused_oid" argument, and other code that can be simplified now that we never have on OID in that context, that'll be cleaned up in subsequent commits, but for now let's narrowly focus on fixing the "git gc" issue. As the modified assert() shows we always pass a NULL oid to reflog_expire() now. Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals: ( rm -rf /tmp/git && git init /tmp/git && while true do head -c 10 /dev/urandom | hexdump >/tmp/git/out && git -C /tmp/git add out && git -C /tmp/git commit -m"out" done ) ( rm -rf /tmp/git-clone && git clone file:///tmp/git /tmp/git-clone && while git -C /tmp/git-clone pull do date done ) ( while git -C /tmp/git-clone reflog expire --all do date done ) Before this change the "reflog expire" would fail really quickly with the "but expected" error noted above. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). As noted above that can in turn be mitigated with higher values of core.filesRefLockTimeout than the 100ms default. As noted in the commentary added in the preceding commit there's also the case of branches being racily deleted, that can be tested by adding this to the above: ( while git -C /tmp/git-clone branch topic master && git -C /tmp/git-clone branch -D topic do date done ) With core.filesRefLockTimeout set to 10 seconds (it can probably be a lot lower) I managed to run all four of these concurrently for about an hour, and accumulated ~125k commits, auto-gc's and all, and didn't have a single failure. The loops visibly stall while waiting for the lock, but that's expected and desired behavior. 0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use 1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/ 2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu 3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
github-actions bot
pushed a commit
that referenced
this pull request
Aug 10, 2021
When doing reference negotiation, git-fetch-pack(1) is loading all refs
from disk in order to determine which commits it has in common with the
remote repository. This can be quite expensive in repositories with many
references though: in a real-world repository with around 2.2 million
refs, fetching a single commit by its ID takes around 44 seconds.
Dominating the loading time is decompression and parsing of the objects
which are referenced by commits. Given the fact that we only care about
commits (or tags which can be peeled to one) in this context, there is
thus an easy performance win by switching the parsing logic to make use
of the commit graph in case we have one available. Like this, we avoid
hitting the object database to parse these commits but instead only load
them from the commit-graph. This results in a significant performance
boost when executing git-fetch in said repository with 2.2 million refs:
Benchmark #1: HEAD~: git fetch $remote $commit
Time (mean ± σ): 44.168 s ± 0.341 s [User: 42.985 s, System: 1.106 s]
Range (min … max): 43.565 s … 44.577 s 10 runs
Benchmark #2: HEAD: git fetch $remote $commit
Time (mean ± σ): 19.498 s ± 0.724 s [User: 18.751 s, System: 0.690 s]
Range (min … max): 18.629 s … 20.454 s 10 runs
Summary
'HEAD: git fetch $remote $commit' ran
2.27 ± 0.09 times faster than 'HEAD~: git fetch $remote $commit'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Aug 27, 2021
In order to compute whether objects reachable from a set of tips are all
connected, we do a revision walk with these tips as positive references
and `--not --all`. `--not --all` will cause the revision walk to load
all preexisting references as uninteresting, which can be very expensive
in repositories with many references.
Benchmarking the git-rev-list(1) command highlights that by far the most
expensive single phase is initial sorting of the input revisions: after
all references have been loaded, we first sort commits by author date.
In a real-world repository with about 2.2 million references, it makes
up about 40% of the total runtime of git-rev-list(1).
Ultimately, the connectivity check shouldn't really bother about the
order of input revisions at all. We only care whether we can actually
walk all objects until we hit the cut-off point. So sorting the input is
a complete waste of time.
Introduce a new "--unsorted-input" flag to git-rev-list(1) which will
cause it to not sort the commits and adjust the connectivity check to
always pass the flag. This results in the following speedups, executed
in a clone of gitlab-org/gitlab [1]:
Benchmark #1: git rev-list --objects --quiet --not --all --not $(cat newrev)
Time (mean ± σ): 7.639 s ± 0.065 s [User: 7.304 s, System: 0.335 s]
Range (min … max): 7.543 s … 7.742 s 10 runs
Benchmark #2: git rev-list --unsorted-input --objects --quiet --not --all --not $newrev
Time (mean ± σ): 4.995 s ± 0.044 s [User: 4.657 s, System: 0.337 s]
Range (min … max): 4.909 s … 5.048 s 10 runs
Summary
'git rev-list --unsorted-input --objects --quiet --not --all --not $(cat newrev)' ran
1.53 ± 0.02 times faster than 'git rev-list --objects --quiet --not --all --not $newrev'
[1]: https://gitlab.com/gitlab-org/gitlab.git. Note that not all refs
are visible to clients.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Aug 27, 2021
When queueing up references for the revision walk, `handle_one_ref()`
will resolve the reference's object ID via `get_reference()` and then
queue the ID as pending object via `add_pending_oid()`. But given that
`add_pending_oid()` is only a thin wrapper around `add_pending_object()`
which fist calls `get_reference()`, we effectively resolve the reference
twice and thus duplicate some of the work.
Fix the issue by instead calling `add_pending_object()` directly, which
takes the already-resolved object as input. In a repository with lots of
refs, this translates into a near 10% speedup:
Benchmark #1: HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev
Time (mean ± σ): 5.015 s ± 0.038 s [User: 4.698 s, System: 0.316 s]
Range (min … max): 4.970 s … 5.089 s 10 runs
Benchmark #2: HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev
Time (mean ± σ): 4.606 s ± 0.029 s [User: 4.260 s, System: 0.345 s]
Range (min … max): 4.565 s … 4.657 s 10 runs
Summary
'HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev' ran
1.09 ± 0.01 times faster than 'HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Aug 27, 2021
When queueing references in git-rev-list(1), we try to optimize parsing
of commits via the commit-graph. To do so, we first look up the object's
type, and if it is a commit we call `repo_parse_commit()` instead of
`parse_object()`. This is quite inefficient though given that we're
always uncompressing the object header in order to determine the type.
Instead, we can opportunistically search the commit-graph for the object
ID: in case it's found, we know it's a commit and can directly fill in
the commit object without having to uncompress the object header.
Expose a new function `lookup_commit_in_graph()`, which tries to find a
commit in the commit-graph by ID, and convert `get_reference()` to use
this function. This provides a big performance win in cases where we
load references in a repository with lots of references pointing to
commits. The following has been executed in a real-world repository with
about 2.2 million refs:
Benchmark #1: HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev
Time (mean ± σ): 4.458 s ± 0.044 s [User: 4.115 s, System: 0.342 s]
Range (min … max): 4.409 s … 4.534 s 10 runs
Benchmark #2: HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev
Time (mean ± σ): 3.089 s ± 0.015 s [User: 2.768 s, System: 0.321 s]
Range (min … max): 3.061 s … 3.105 s 10 runs
Summary
'HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev' ran
1.44 ± 0.02 times faster than 'HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
github-actions bot
pushed a commit
that referenced
this pull request
Sep 2, 2021
When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.
While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.
Fix this issue by completely skipping both preparation of the format and
formatting data for display in case the user passes `--quiet`, improving
performance especially with many refs. The following benchmark shows a
nice speedup for a quiet mirror-fetch in a repository with 2.3M refs:
Benchmark #1: HEAD~: git-fetch
Time (mean ± σ): 26.929 s ± 0.145 s [User: 24.194 s, System: 4.656 s]
Range (min … max): 26.692 s … 27.068 s 5 runs
Benchmark #2: HEAD: git-fetch
Time (mean ± σ): 25.189 s ± 0.094 s [User: 22.556 s, System: 4.606 s]
Range (min … max): 25.070 s … 25.314 s 5 runs
Summary
'HEAD: git-fetch' ran
1.07 ± 0.01 times faster than 'HEAD~: git-fetch'
While at it, this patch also fixes `adjust_refcol_width()` such that it
skips unchanged refs in case the user passed `--quiet`, where verbosity
will be negative. While this function won't be called anymore if so,
this brings the comment in line with actual code. Furthermore, needless
`verbosity >= 0` checks are now removed in `store_updated_refs()`: we
never print to the `note` buffer anymore in case `verbosity < 0`, so we
won't end up in that code block anyway.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Sep 18, 2021
When updating our local refs based on the refs fetched from the remote,
we need to iterate through all requested refs and load their respective
commits such that we can determine whether they need to be appended to
FETCH_HEAD or not. In cases where we're fetching from a remote with
exceedingly many refs, resolving these refs can be quite expensive given
that we repeatedly need to unpack object headers for each of the
referenced objects.
Speed this up by opportunistically trying to resolve object IDs via the
commit graph. We only do so for any refs which are not in "refs/tags":
more likely than not, these are going to be a commit anyway, and this
lets us avoid having to unpack object headers completely in case the
object is a commit that is part of the commit-graph. This significantly
speeds up mirror-fetches in a real-world repository with
2.3M refs:
Benchmark #1: HEAD~: git-fetch
Time (mean ± σ): 56.482 s ± 0.384 s [User: 53.340 s, System: 5.365 s]
Range (min … max): 56.050 s … 57.045 s 5 runs
Benchmark #2: HEAD: git-fetch
Time (mean ± σ): 33.727 s ± 0.170 s [User: 30.252 s, System: 5.194 s]
Range (min … max): 33.452 s … 33.871 s 5 runs
Summary
'HEAD: git-fetch' ran
1.67 ± 0.01 times faster than 'HEAD~: git-fetch'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Sep 18, 2021
When updating local refs after the fetch has transferred all objects, we
do an object existence test as a safety guard to avoid updating a ref to
an object which we don't have. We do so via `oid_object_info()`: if it
returns an error, then we know the object does not exist.
One side effect of `oid_object_info()` is that it parses the object's
type, and to do so it must unpack the object header. This is completely
pointless: we don't care for the type, but only want to assert that the
object exists.
Refactor the code to use `repo_has_object_file()`, which both makes the
code's intent clearer and is also faster because it does not unpack
object headers. In a real-world repo with 2.3M refs, this results in a
small speedup when doing a mirror-fetch:
Benchmark #1: HEAD~: git-fetch
Time (mean ± σ): 33.686 s ± 0.176 s [User: 30.119 s, System: 5.262 s]
Range (min … max): 33.512 s … 33.944 s 5 runs
Benchmark #2: HEAD: git-fetch
Time (mean ± σ): 31.247 s ± 0.195 s [User: 28.135 s, System: 5.066 s]
Range (min … max): 30.948 s … 31.472 s 5 runs
Summary
'HEAD: git-fetch' ran
1.08 ± 0.01 times faster than 'HEAD~: git-fetch'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Sep 18, 2021
The object ID iterator used by the connectivity checks returns the next
object ID via an out-parameter and then uses a return code to indicate
whether an item was found. This is a bit roundabout: instead of a
separate error code, we can just return the next object ID directly and
use `NULL` pointers as indicator that the iterator got no items left.
Furthermore, this avoids a copy of the object ID.
Refactor the iterator and all its implementations to return object IDs
directly. This brings a tiny performance improvement when doing a mirror-fetch of a repository with about 2.3M refs:
Benchmark #1: 328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch
Time (mean ± σ): 30.110 s ± 0.148 s [User: 27.161 s, System: 5.075 s]
Range (min … max): 29.934 s … 30.406 s 10 runs
Benchmark #2: 328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch
Time (mean ± σ): 29.899 s ± 0.109 s [User: 26.916 s, System: 5.104 s]
Range (min … max): 29.696 s … 29.996 s 10 runs
Summary
'328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch' ran
1.01 ± 0.01 times faster than '328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch'
While this 1% speedup could be labelled as statistically insignificant,
the speedup is consistent on my machine. Furthermore, this is an end to
end test, so it is expected that the improvement in the connectivity
check itself is more significant.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Sep 18, 2021
In order to negotiate a packfile, we need to dereference refs to see
which commits we have in common with the remote. To do so, we first look
up the object's type -- if it's a tag, we peel until we hit a non-tag
object. If we hit a commit eventually, then we return that commit.
In case the object ID points to a commit directly, we can avoid the
initial lookup of the object type by opportunistically looking up the
commit via the commit-graph, if available, which gives us a slight speed
bump of about 2% in a huge repository with about 2.3M refs:
Benchmark #1: HEAD~: git-fetch
Time (mean ± σ): 31.634 s ± 0.258 s [User: 28.400 s, System: 5.090 s]
Range (min … max): 31.280 s … 31.896 s 5 runs
Benchmark #2: HEAD: git-fetch
Time (mean ± σ): 31.129 s ± 0.543 s [User: 27.976 s, System: 5.056 s]
Range (min … max): 30.172 s … 31.479 s 5 runs
Summary
'HEAD: git-fetch' ran
1.02 ± 0.02 times faster than 'HEAD~: git-fetch'
In case this fails, we fall back to the old code which peels the
objects to a commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
pushed a commit
that referenced
this pull request
Sep 18, 2021
When fetching refs, we are doing two connectivity checks:
- The first one is done such that we can skip fetching refs in the
case where we already have all objects referenced by the updated
set of refs.
- The second one verifies that we have all objects after we have
fetched objects.
We always execute both connectivity checks, but this is wasteful in case
the first connectivity check already notices that we have all objects
locally available.
Skip the second connectivity check in case we already had all objects
available. This gives us a nice speedup when doing a mirror-fetch in a
repository with about 2.3M refs where the fetching repo already has all
objects:
Benchmark #1: HEAD~: git-fetch
Time (mean ± σ): 30.025 s ± 0.081 s [User: 27.070 s, System: 4.933 s]
Range (min … max): 29.900 s … 30.111 s 5 runs
Benchmark #2: HEAD: git-fetch
Time (mean ± σ): 25.574 s ± 0.177 s [User: 22.855 s, System: 4.683 s]
Range (min … max): 25.399 s … 25.765 s 5 runs
Summary
'HEAD: git-fetch' ran
1.17 ± 0.01 times faster than 'HEAD~: git-fetch'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ahunt
added a commit
that referenced
this pull request
Sep 18, 2021
cmd_show populate rev with lots of data, and later returns without
cleaning it up. That's reasonable - we need this data until the process
completes - so we take the easy way out and UNLEAK it.
We have to add the UNLEAK early on, as cmd_show might return via
cmd_log_walk() in the next few lines, or it might continue to the
no-walk implementation below.
This patch silences the following leaks which were found when running
t0000 against LSAN:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x83cced in add_object_array_with_path /home/ahunt/oss-fuzz/git/object.c:349:17
#3 0x8f4f5a in add_pending_object_with_path /home/ahunt/oss-fuzz/git/revision.c:329:2
#4 0x8eb2b6 in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2082:2
#5 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12
#6 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7
#7 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9
git#8 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
git#9 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
git#10 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
git#11 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
git#12 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#13 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#14 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#15 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x49a9d2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
#1 0x9ab4c2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8
#2 0x59c269 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:233:18
#3 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
#4 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
#5 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#6 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#7 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#8 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#9 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#10 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8f5e30 in add_rev_cmdline /home/ahunt/oss-fuzz/git/revision.c:1482:23
#3 0x8eb26d in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2081:2
#4 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12
#5 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7
#6 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9
#7 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
git#8 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
git#9 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
git#10 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
git#11 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#12 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#13 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#14 0x7fc4b3f06349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
ahunt
added a commit
that referenced
this pull request
Sep 18, 2021
cmd_show() uses objects to point to rev.pending's objects. Later, it might
detach rev.pending's objects: when handling an OBJ_COMMIT it will reset
rev.pending (without freeing its objects). Detaching (as opposed to freeing)
is necessary because cmd_show() continues iterating over the original objects
array.
We choose to UNLEAK because there's no real advantage to cleaning up
properly (cmd_show() exits immediately after looping over these
objects). A number of alternatives exist, but are all significantly more
complex for no gain:
Alternative 1:
Convert objects into an object_array, and memcpy rev.pending into it
(followed by detaching rev.pending immediately - making objects the
owner of what used to be rev.pending). Then we could safely
objects_array_clear() at the end of cmd_show(). And we can rely on
a preexisting UNLEAK(rev) to avoid having to clean up rev.pending.
This is a more complex and riskier approach vs a simple UNLEAK,
and doesn't add value.
Alternative 2:
A variation on alternative 1. We make objects own the object_array as
before. Once we're done, we free the new rev.pending array (which
might be empty), and we memcpy objects back into rev.pending, relying
on the existin UNLEAK(rev) to avoid having to free rev.pending.
ASAN output from t0000:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x487504 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:437:3
#1 0x9e4ef8 in xstrdup wrapper.c:29:14
#2 0x86395d in add_object_array_with_path object.c:366:17
#3 0x9264fc in add_pending_object_with_path revision.c:330:2
#4 0x91c4e0 in handle_revision_arg_1 revision.c:2086:2
#5 0x91bfcd in handle_revision_arg revision.c:2093:12
#6 0x91ff5a in setup_revisions revision.c:2780:7
#7 0x5a7678 in cmd_log_init_finish builtin/log.c:206:9
git#8 0x5a4f18 in cmd_log_init builtin/log.c:278:2
git#9 0x5a55d1 in cmd_show builtin/log.c:646:2
git#10 0x4cff30 in run_builtin git.c:461:11
git#11 0x4cdb00 in handle_builtin git.c:713:3
git#12 0x4cf527 in run_argv git.c:780:4
git#13 0x4cd426 in cmd_main git.c:911:19
git#14 0x6b2eb5 in main common-main.c:52:11
git#15 0x7f74fc9bd349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 41 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
ahunt
added a commit
that referenced
this pull request
Sep 18, 2021
cmd_show puts a lot of data into rev, and doesn't clean it up before
returning. That's reasonable - we use most if not all of rev up until
cmd_show is finished - there's not much value in doing a proper cleanup.
Therefore we take the easy way out and UNLEAK rev.
The UNLEAK has to be performed early on, as cmd_show might return via
cmd_log_walk() in the next few lines, or it might continue to the
no-walk implementation below.
This patch silences the following leaks which were found when running
t0000 against LSAN:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x83cced in add_object_array_with_path /home/ahunt/oss-fuzz/git/object.c:349:17
#3 0x8f4f5a in add_pending_object_with_path /home/ahunt/oss-fuzz/git/revision.c:329:2
#4 0x8eb2b6 in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2082:2
#5 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12
#6 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7
#7 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9
git#8 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
git#9 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
git#10 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
git#11 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
git#12 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#13 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#14 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#15 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x49a9d2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
#1 0x9ab4c2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8
#2 0x59c269 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:233:18
#3 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
#4 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
#5 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#6 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#7 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#8 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#9 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#10 0x7f7c56197349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Indirect leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab168 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8f5e30 in add_rev_cmdline /home/ahunt/oss-fuzz/git/revision.c:1482:23
#3 0x8eb26d in handle_revision_arg_1 /home/ahunt/oss-fuzz/git/revision.c:2081:2
#4 0x8eadad in handle_revision_arg /home/ahunt/oss-fuzz/git/revision.c:2089:12
#5 0x8eea99 in setup_revisions /home/ahunt/oss-fuzz/git/revision.c:2756:7
#6 0x59c024 in cmd_log_init_finish /home/ahunt/oss-fuzz/git/builtin/log.c:206:9
#7 0x5998d8 in cmd_log_init /home/ahunt/oss-fuzz/git/builtin/log.c:275:2
git#8 0x599f9b in cmd_show /home/ahunt/oss-fuzz/git/builtin/log.c:641:2
git#9 0x4cd92d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
git#10 0x4cb5fa in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
git#11 0x4ccf57 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
git#12 0x4caf49 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
git#13 0x69ce3e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
git#14 0x7fc4b3f06349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
ahunt
added a commit
that referenced
this pull request
Sep 18, 2021
cmd_show() uses objects to point to rev.pending's objects. Later, it might
detach rev.pending's objects: when handling an OBJ_COMMIT it will reset
rev.pending (without freeing its objects). Detaching (as opposed to freeing)
is necessary because cmd_show() continues iterating over the original objects
array.
We choose to UNLEAK because there's no real advantage to cleaning up
properly (cmd_show() exits immediately after looping over these
objects). A number of alternatives exist, but are all significantly more
complex for no gain:
Alternative 1:
Convert objects into an object_array, and memcpy rev.pending into it
(followed by detaching rev.pending immediately - making objects the
owner of what used to be rev.pending). Then we could safely
objects_array_clear() at the end of cmd_show(). And we can rely on
a preexisting UNLEAK(rev) to avoid having to clean up rev.pending.
This is a more complex and riskier approach vs a simple UNLEAK,
and doesn't add any user-visible value.
Alternative 2:
A variation on alternative 1. We make objects own the object_array as
before. Once we're done, we free the new rev.pending array (which
might be empty), and we memcpy objects back into rev.pending, relying
on the existin UNLEAK(rev) to avoid having to free rev.pending.
ASAN output from t0000:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x487504 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:437:3
#1 0x9e4ef8 in xstrdup wrapper.c:29:14
#2 0x86395d in add_object_array_with_path object.c:366:17
#3 0x9264fc in add_pending_object_with_path revision.c:330:2
#4 0x91c4e0 in handle_revision_arg_1 revision.c:2086:2
#5 0x91bfcd in handle_revision_arg revision.c:2093:12
#6 0x91ff5a in setup_revisions revision.c:2780:7
#7 0x5a7678 in cmd_log_init_finish builtin/log.c:206:9
git#8 0x5a4f18 in cmd_log_init builtin/log.c:278:2
git#9 0x5a55d1 in cmd_show builtin/log.c:646:2
git#10 0x4cff30 in run_builtin git.c:461:11
git#11 0x4cdb00 in handle_builtin git.c:713:3
git#12 0x4cf527 in run_argv git.c:780:4
git#13 0x4cd426 in cmd_main git.c:911:19
git#14 0x6b2eb5 in main common-main.c:52:11
git#15 0x7f74fc9bd349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 41 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
ahunt
pushed a commit
that referenced
this pull request
Sep 26, 2021
During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one and expires it. This behavior has been with us since this command was implemented in 4264dc1 ("git reflog expire", 2006-12-19). Change this to stop calling lock_ref_oid_basic() with the OID we saw when we looped over the logs, instead have it pass the OID it managed to lock. This mostly mitigates a race condition where e.g. "git gc" will fail in a concurrently updated repository because the branch moved since "git reflog expire --all" was started. I.e. with: error: cannot lock ref '<refname>': ref '<refname>' is at <OID-A> but expected <OID-B> This behavior of passing in an "oid" was needed for an edge-case that I've untangled in this and preceding commits though, namely that we needed this OID because we'd: 1. Lookup the reflog name/OID via dwim_log() 2. With that OID, lock the reflog 3. Later in builtin/reflog.c we use the OID we looked as input to lookup_commit_reference_gently(), assured that it's equal to the OID we got from dwim_log(). We can be sure that this change is safe to make because between dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other logic relevant to the OID or expiry run in the cmd_reflog_expire() caller. We can thus treat that code as a black box, before and after this change it would get an OID that's been locked, the only difference is that now we mostly won't be failing to get the lock due to the TOCTOU race[0]. That failure was purely an implementation detail in how the "current OID" was looked up, it was divorced from the locking mechanism. What do we mean with "mostly"? It mostly mitigates it because we'll still run into cases where the ref is locked and being updated as we want to expire it, and other git processes wanting to update the refs will in turn race with us as we expire the reflog. That remaining race can in turn be mitigated with the core.filesRefLockTimeout setting, see 4ff0f01 ("refs: retry acquiring reference locks for 100ms", 2017-08-21). In practice if that value is high enough we'll probably never have ref updates or reflog expiry failing, since the clients involved will retry for far longer than the time any of those operations could take. See [1] for an initial report of how this impacted "git gc" and a large discussion about this change in early 2019. In particular patch looked good to Michael Haggerty, see his[2]. That message seems to not have made it to the ML archive, its content is quoted in full in my [3]. I'm leaving behind now-unused code the refs API etc. that takes the now-NULL "unused_oid" argument, and other code that can be simplified now that we never have on OID in that context, that'll be cleaned up in subsequent commits, but for now let's narrowly focus on fixing the "git gc" issue. As the modified assert() shows we always pass a NULL oid to reflog_expire() now. Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals: ( rm -rf /tmp/git && git init /tmp/git && while true do head -c 10 /dev/urandom | hexdump >/tmp/git/out && git -C /tmp/git add out && git -C /tmp/git commit -m"out" done ) ( rm -rf /tmp/git-clone && git clone file:///tmp/git /tmp/git-clone && while git -C /tmp/git-clone pull do date done ) ( while git -C /tmp/git-clone reflog expire --all do date done ) Before this change the "reflog expire" would fail really quickly with the "but expected" error noted above. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). As noted above that can in turn be mitigated with higher values of core.filesRefLockTimeout than the 100ms default. As noted in the commentary added in the preceding commit there's also the case of branches being racily deleted, that can be tested by adding this to the above: ( while git -C /tmp/git-clone branch topic master && git -C /tmp/git-clone branch -D topic do date done ) With core.filesRefLockTimeout set to 10 seconds (it can probably be a lot lower) I managed to run all four of these concurrently for about an hour, and accumulated ~125k commits, auto-gc's and all, and didn't have a single failure. The loops visibly stall while waiting for the lock, but that's expected and desired behavior. 0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use 1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/ 2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu 3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.