Repository navigation
Conversation
diff_file_content_load_workdir_file() mapped unfiltered working directory files with git_futils_mmap_ro(). If another process truncates such a file while it is mapped (an editor or tool rewriting it in place), the next access to a page past the new end of file raises SIGBUS and kills the process; no error can be returned. Both xdiff and the OID computation read the mapping after it is created. Always load the file with git_futils_readbuffer_fd(), which was already used for filtered files and as the fallback when mmap fails. A file truncated during the read now produces an error instead. Remove GIT_DIFF_FLAG__UNMAP_DATA, which nothing sets any more.
This branch has not been deployed
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
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.
diff_file_content_load_workdir_file()maps unfiltered working-directory files withgit_futils_mmap_ro(). If another process truncates such a file while it is mapped (an editor or tool rewriting it in place), the next access to a page past the new end of file raises SIGBUS and kills the whole process. A caller can't handle it: no error is returned, and catching SIGBUS partway through a libgit2 call isn't safe. xdiff (xdl_prepare_env→xdl_hash_record/xdl_recmatch) and the OID computation (git_odb__hash→SHA1DCUpdate) both read the mapping after it is created.We hit this in production in an application that computes status and diffs while coding agents rewrite files in the working tree. The two crash stacks we saw were:
git_patch_from_diff→git_xdiff→xdl_diff→xdl_prepare_env→xdl_recmatchdiff_file_content_load_workdir→git_odb__hash→git_hash_vec→SHA1DCUpdateChange
Always load the file with
git_futils_readbuffer_fd(). That is the path libgit2 already takes for filtered files, and the fallback it uses when mmap fails. If the file is truncated during the read, the call now fails with an ordinary error ("could not read (expected N bytes, read M)") instead of crashing.GIT_DIFF_FLAG__UNMAP_DATAis no longer set anywhere, so it is removed.The output bytes cannot change: with no filters,
git_filter_list__convert_buf()just moves the buffer.The other mappings in libgit2 are all of files inside
.git: pack windows, indexes, the commit-graph, packed-refs and loose-object streams. Those are replaced by writing a new file and renaming it into place, not by truncating the old one, so they are not affected.Reproduction
The reproducer below commits a 4 MiB file. A forked child then keeps truncating that file and rewriting it in place, while the parent loops over
git_diff_index_to_workdir()+git_patch_from_diff()for 10 seconds.Results, 5 runs per row:
main@ 0551dfd#7315 (private mappings) makes the crash less frequent on macOS but does not fix it. A private mapping still faults when the file underneath it is truncated.
repro.c
Build it with
cc -o repro repro.c $(pkg-config --cflags --static --libs libgit2)and runmkdir r && ./repro r.Tests
The full default
libgit2_testssuite passes on macOS.This PR doesn't add a unit test, because I couldn't find a deterministic one. The public API has no callback between loading the content and running xdiff, so a test can't truncate the file inside that window.
I tried truncating from the first hunk callback, after xdiff has already read the whole mapping. That did not fault on either Linux or macOS once the pages were resident, so it passed with and without the fix. That's why I didn't include it.
A
fork()-based stress test like the reproducer would be timing-dependent and wouldn't run on Windows. I'm happy to add one if you'd prefer that, or a test hook indiff_file.c.Cost
The file is copied once into heap memory instead of being mapped. libgit2 already does this for every filtered file. Files above the big-file threshold are treated as binary by size and never loaded. In our application, diffing about 3000 modified files (roughly 88 MB of working-tree reads per diff) showed no measurable timing difference.