From bd871e9d2a80a553a847c924a490f45454aa45ea Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 10 Nov 2026 10:58:11 -0800 Subject: [PATCH 1/2] Expand GVFS command-lock exemptions Exempt reflog and audited ref, object, and reader commands. Classify describe, grep, merge-file, and refs modes conservatively. Keep locks for unknown options and modes that can change tracked files, refresh the index, or dispatch arbitrary commands. Add policy tests for both hook phases and adversarial option forms. Add mounted tests for contention, placeholder creation, and index preservation during reflog writes. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella --- .../GitReadAndGitLockTests.cs | 110 ++++++++ .../GitCommands/CreatePlaceholderTests.cs | 11 +- GVFS/GVFS.Hooks/Program.CommandLocking.cs | 267 +++++++++++++++++- .../Hooks/CommandLockingTests.cs | 98 ++++++- 4 files changed, 481 insertions(+), 5 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index 0427576b7..55fa5b06f 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -110,6 +110,21 @@ public void GitCommandWaitsWhileAnotherIsRunning(string command) [TestCase("count-objects -vH")] [TestCase("ls-remote --get-url origin")] [TestCase("range-diff HEAD~1..HEAD HEAD~1..HEAD")] + [TestCase("reflog")] + [TestCase("reflog show --all")] + [TestCase("reflog list")] + [TestCase("reflog exists HEAD")] + [TestCase("describe --always HEAD")] + [TestCase("grep -n -e . HEAD -- Readme.md")] + [TestCase("grep --cached -e . -- Readme.md")] + [TestCase("refs list --count=1")] + [TestCase("merge-file -p .git/HEAD .git/HEAD .git/HEAD")] + [TestCase("merge-file --object-id HEAD:Readme.md HEAD:Readme.md HEAD:Readme.md")] + [TestCase("merge-tree --write-tree HEAD HEAD")] + [TestCase("notes list")] + [TestCase("replace -l")] + [TestCase("format-patch --stdout -1 HEAD")] + [TestCase("var GIT_AUTHOR_IDENT")] [TestCase("shortlog -sn HEAD")] [TestCase("show-branch --list")] public void ReadOnlyCommandDoesNotWaitForLock(string command) @@ -128,6 +143,101 @@ public void ReadOnlyCommandDoesNotWaitForLock(string command) }); } + [TestCase("describe --dirty --always", 0)] + [TestCase("describe --broken --always", 0)] + [TestCase("describe --di --always", 0)] + [TestCase("grep -nOcat -e . -- Readme.md", 0)] + [TestCase("grep --open=cat -e . -- Readme.md", 0)] + [TestCase("refs migrate -h", 0)] + public void UnsafeCommandModeStillWaitsForLock(string command, int expectedExitCode) + { + File.ReadAllText(this.Enlistment.GetVirtualPathTo("Readme.md")); + ProcessResult result = this.RunWithLockHolder(command); + result.ExitCode.ShouldEqual(expectedExitCode, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase(false)] + [TestCase(true)] + public void MergeFileOnlyBypassesLockWhenOutputModeRemainsEnabled(bool disableStdout) + { + string inputPath = Path.Combine(this.Enlistment.EnlistmentRoot, "merge-file-" + Guid.NewGuid().ToString("N")); + File.WriteAllText(inputPath, "merge input\n"); + try + { + string command = $"merge-file --stdout {(disableStdout ? "--no-stdout" : string.Empty)} \"{inputPath}\" \"{inputPath}\" \"{inputPath}\""; + ProcessResult result = this.RunWithLockHolder(command, shouldWait: disableStdout); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.Contains(ExpectedStatusWaitingText).ShouldEqual(disableStdout); + File.ReadAllText(inputPath).ShouldEqual("merge input\n"); + } + finally + { + File.Delete(inputPath); + } + } + + [TestCase] + public void ReflogWritesPreserveIndexAndTrackedContentUnderLockContention() + { + string refName = "refs/heads/gvfs-reflog-test-" + Guid.NewGuid().ToString("N"); + string filePath = this.Enlistment.GetVirtualPathTo("Readme.md"); + string indexPath = this.Enlistment.GetDotGitPath("index"); + string originalContent = File.ReadAllText(filePath); + string oldObject = GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse HEAD~1").Trim(); + string newObject = GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse HEAD").Trim(); + try + { + File.WriteAllText(filePath, originalContent + "\nstaged reflog test\n"); + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "add -- Readme.md"); + string dirtyContent = originalContent + "\nunstaged reflog test\n"; + File.WriteAllText(filePath, dirtyContent); + GitHelpers.CheckGitCommandAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"update-ref --create-reflog -m first {refName} {oldObject}"); + GitHelpers.CheckGitCommandAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"update-ref -m second {refName} {newObject}"); + + GitHelpers.RunWithGVFSLock( + this.Enlistment, + () => + { + byte[] index = File.ReadAllBytes(indexPath); + DateTime indexWriteTime = File.GetLastWriteTimeUtc(indexPath); + string[] commands = + { + $"reflog write {refName} {newObject} {newObject} appended", + $"reflog delete --rewrite --updateref {refName}@{{0}}", + $"reflog delete --rewrite --updateref {refName}@{{0}}", + $"reflog expire --stale-fix --rewrite --updateref --expire=never --expire-unreachable=never {refName}", + $"reflog expire --stale-fix --rewrite --updateref --expire=all {refName}", + $"reflog drop {refName}", + }; + + foreach (string command in commands) + { + ProcessResult result = GitProcess.InvokeProcess( + this.Enlistment.RepoRoot, + command, + timeoutMs: AcquireGVFSLockTimeout); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldNotContain(false, ExpectedStatusWaitingText); + File.ReadAllBytes(indexPath).ShouldMatchInOrder(index); + File.GetLastWriteTimeUtc(indexPath).ShouldEqual(indexWriteTime); + File.ReadAllText(filePath).ShouldEqual(dirtyContent); + } + + GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse " + refName).Trim().ShouldEqual(oldObject); + }); + } + finally + { + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "reset --hard"); + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "update-ref -d " + refName); + } + } + [TestCase, Order(5)] public void GitAliasNamedAfterKnownCommandAcquiresLock() { diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs index 01b9ce351..802966dee 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs @@ -41,6 +41,7 @@ public void AllowsPlaceholderCreationForDefaultAnnotate() [TestCase("hash-object --stdin")] [TestCase("index-pack --stdin")] [TestCase("name-rev --stdin")] + [TestCase("notes copy --stdin")] [TestCase("rev-list --stdin --quiet --all")] [TestCase("update-ref --stdin")] public void AllowsPlaceholderCreationWhileGitCommandIsRunning(string commandToRun) @@ -50,7 +51,6 @@ public void AllowsPlaceholderCreationWhileGitCommandIsRunning(string commandToRu [TestCase("checkout-index --stdin")] [TestCase("fetch-pack --stdin URL")] - [TestCase("notes copy --stdin")] [TestCase("reset --stdin")] [TestCase("send-pack --stdin URL")] [TestCase("update-index --stdin")] @@ -62,11 +62,16 @@ public void BlocksPlaceholderCreationWhileGitCommandIsRunning(string commandToRu [TestCase("worktree list")] [TestCase("worktree list --porcelain")] [TestCase("worktree list --porcelain -z")] - public void AllowsPlaceholderCreationWhileWorktreeListIsRunning(string command) + [TestCase("reflog")] + [TestCase("describe --always HEAD")] + [TestCase("grep -q -e . HEAD -- Readme.md")] + [TestCase("refs list --count=1")] + [TestCase("merge-file --object-id HEAD:Readme.md HEAD:Readme.md HEAD:Readme.md")] + public void AllowsPlaceholderCreationWhileExemptCommandIsRunning(string command) { string hookListPath = this.Enlistment.GetVirtualPathTo(".git", "hooks", "post-command.hooks"); string originalHookList = File.ReadAllText(hookListPath); - string eventName = "GVFS.WorktreeList." + Guid.NewGuid().ToString("N"); + string eventName = "GVFS.ExemptCommand." + Guid.NewGuid().ToString("N"); string expectedContent = File.ReadAllText(Path.Combine(this.ControlGitRepo.RootPath, FileToRead)); using (EventWaitHandle ready = new EventWaitHandle(false, EventResetMode.ManualReset, eventName + ".Ready")) diff --git a/GVFS/GVFS.Hooks/Program.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs index b016a7d9e..292575c80 100644 --- a/GVFS/GVFS.Hooks/Program.CommandLocking.cs +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -11,7 +11,8 @@ public partial class Program // The lock serializes changes to the enlistment's index and tracked working tree with GVFS projection updates. // Before adding an exemption, check all supported options, subcommands, and child-command paths. - // Ref and object writes alone do not require this lock. Index refreshes do. + // Ref and object writes alone do not require this lock. + // Existing diff exemptions allow opportunistic stat-cache refreshes. // Keep arbitrary Git dispatchers locked because nested command hooks can be suppressed. // Excluded readers can observe concurrent working-tree changes. They do not receive snapshot isolation. internal static bool TryShouldLock( @@ -55,47 +56,70 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) case "annotate": case "blame": case "branch": + case "bundle": case "cat-file": case "check-attr": case "check-ignore": case "check-mailmap": case "check-ref-format": case "cherry": + case "column": case "commit-graph": + case "commit-tree": case "config": case "count-objects": case "credential": case "diff": case "diff-files": case "diff-index": + case "diff-pairs": case "diff-tree": case "difftool": case "fetch": + case "fmt-merge-msg": case "for-each-ref": + case "format-patch": + case "format-rev": + case "get-tar-commit-id": case "help": case "hash-object": case "index-pack": + case "last-modified": case "log": case "ls-files": case "ls-remote": case "ls-tree": case "merge-base": + case "merge-tree": + case "mktag": + case "mktree": case "multi-pack-index": case "name-rev": + case "notes": case "pack-objects": + case "pack-refs": + case "patch-id": + case "pickaxe": case "push": case "range-diff": + case "reflog": case "remote": + case "replace": case "rev-list": case "rev-parse": case "shortlog": case "show": case "show-branch": + case "show-index": case "show-ref": + case "stripspace": case "symbolic-ref": case "tag": case "unpack-objects": case "update-ref": + case "update-server-info": + case "url-parse": + case "var": case "verify-commit": case "verify-pack": case "verify-tag": @@ -103,6 +127,31 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) case "web--browse": return false; + case "describe": + return !HasOnlySafeOptions(args, GetDescribeOptionType, out _); + + case "grep": + return !HasOnlySafeOptions(args, GetGrepOptionType, out _); + + case "merge-file": + return !HasOnlySafeOptions(args, GetMergeFileOptionType, out bool hasReadOnlyMode) || !hasReadOnlyMode; + + case "refs": + switch (args.Length > 2 ? args[2] : null) + { + case "list": + case "exists": + case "verify": + case "optimize": + case "create": + case "delete": + case "update": + case "rename": + return false; + } + + return true; + case "worktree": return WorktreeCommandParser.GetSubcommand(args) != "list"; @@ -127,6 +176,213 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) return true; } + private static bool HasOnlySafeOptions(string[] args, Func getOptionType, out bool hasReadOnlyMode) + { + hasReadOnlyMode = false; + int end = args.Length; + + // Only the appended hook metadata is excluded. User arguments with these names remain unknown options. + if (end > 2 && args[0] == "post-command" && args[end - 1].StartsWith("--exit_code=", StringComparison.Ordinal)) + { + --end; + } + + if (end > 2 && args[end - 1].StartsWith("--git-pid=", StringComparison.Ordinal)) + { + --end; + } + + for (int i = 2; i < end; ++i) + { + string argument = args[i]; + if (argument == "--") + { + return true; + } + + if (!argument.StartsWith("-", StringComparison.Ordinal) || argument == "-") + { + continue; + } + + if (argument.StartsWith("--", StringComparison.Ordinal)) + { + int equals = argument.IndexOf('='); + string option = equals < 0 ? argument : argument.Substring(0, equals); + CommandOptionType type = getOptionType(option); + if (type == CommandOptionType.Unknown || + (equals >= 0 && type != CommandOptionType.Value && type != CommandOptionType.OptionalValue)) + { + return false; + } + + hasReadOnlyMode |= type == CommandOptionType.ReadOnlyMode; + if (type == CommandOptionType.Value && equals < 0 && ++i >= end) + { + return false; + } + } + else + { + for (int j = 1; j < argument.Length; ++j) + { + CommandOptionType type = getOptionType("-" + argument[j]); + if (type == CommandOptionType.Unknown) + { + return false; + } + + hasReadOnlyMode |= type == CommandOptionType.ReadOnlyMode; + if (type == CommandOptionType.Value || type == CommandOptionType.OptionalValue) + { + if (type == CommandOptionType.Value && j == argument.Length - 1 && ++i >= end) + { + return false; + } + + break; + } + } + } + } + + return true; + } + + private static CommandOptionType GetDescribeOptionType(string option) + { + switch (option) + { + case "--contains": + case "--debug": + case "--all": + case "--tags": + case "--long": + case "--first-parent": + case "--exact-match": + case "--always": + return CommandOptionType.Flag; + + case "--candidates": + case "--match": + case "--exclude": + return CommandOptionType.Value; + + case "--abbrev": + return CommandOptionType.OptionalValue; + } + + return CommandOptionType.Unknown; + } + + private static CommandOptionType GetGrepOptionType(string option) + { + switch (option) + { + case "-v": + case "-i": + case "-w": + case "-a": + case "-I": + case "-r": + case "-E": + case "-G": + case "-F": + case "-P": + case "-n": + case "-h": + case "-H": + case "-l": + case "-L": + case "-z": + case "-o": + case "-c": + case "-p": + case "-W": + case "-q": + case "--cached": + case "--no-index": + case "--untracked": + case "--exclude-standard": + case "--recurse-submodules": + case "--invert-match": + case "--ignore-case": + case "--word-regexp": + case "--text": + case "--textconv": + case "--recursive": + case "--extended-regexp": + case "--basic-regexp": + case "--fixed-strings": + case "--perl-regexp": + case "--line-number": + case "--column": + case "--full-name": + case "--files-with-matches": + case "--name-only": + case "--files-without-match": + case "--null": + case "--only-matching": + case "--count": + case "--break": + case "--heading": + case "--show-function": + case "--function-context": + case "--and": + case "--or": + case "--not": + case "--quiet": + case "--all-match": + return CommandOptionType.Flag; + + case "-e": + case "-f": + case "-A": + case "-B": + case "-C": + case "-m": + case "--max-depth": + case "--context": + case "--before-context": + case "--after-context": + case "--threads": + case "--max-count": + return CommandOptionType.Value; + + case "--color": + return CommandOptionType.OptionalValue; + } + + return CommandOptionType.Unknown; + } + + private static CommandOptionType GetMergeFileOptionType(string option) + { + switch (option) + { + case "-p": + case "--stdout": + case "--object-id": + return CommandOptionType.ReadOnlyMode; + + case "-q": + case "--quiet": + case "--diff3": + case "--zdiff3": + case "--ours": + case "--theirs": + case "--union": + return CommandOptionType.Flag; + + case "-L": + case "--diff-algorithm": + case "--marker-size": + return CommandOptionType.Value; + } + + return CommandOptionType.Unknown; + } + private static string GetGitCommand(string[] args) { string command = args[1].ToLowerInvariant(); @@ -137,5 +393,14 @@ private static string GetGitCommand(string[] args) return command; } + + private enum CommandOptionType + { + Unknown, + Flag, + Value, + OptionalValue, + ReadOnlyMode, + } } } diff --git a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs index 433ee028e..1fe855a7b 100644 --- a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs @@ -13,6 +13,7 @@ public class CommandLockingTests [TestCase("annotate --contents mock-file HEAD")] [TestCase("blame")] [TestCase("branch")] + [TestCase("bundle create mock.bundle HEAD")] [TestCase("cat-file")] [TestCase("check-attr")] [TestCase("check-ignore")] @@ -21,33 +22,86 @@ public class CommandLockingTests [TestCase("check-ref-format --normalize refs/heads/topic")] [TestCase("check-ref-format --refspec-pattern refs/heads/*")] [TestCase("cherry -v HEAD~2 HEAD HEAD~1")] + [TestCase("column --mode=column")] [TestCase("commit-graph")] + [TestCase("commit-tree HEAD^{tree}")] [TestCase("config")] [TestCase("count-objects")] [TestCase("count-objects -vH")] [TestCase("credential")] + [TestCase("describe")] + [TestCase("describe --tags --always --abbrev=7 HEAD")] + [TestCase("describe --contains --all --long --first-parent --exact-match")] + [TestCase("describe --match mock-* --exclude=excluded-* --candidates 5")] + [TestCase("describe --match --dirty")] + [TestCase("describe -- --dirty")] [TestCase("diff")] [TestCase("diff-files")] [TestCase("diff-index")] + [TestCase("diff-pairs")] [TestCase("diff-tree")] [TestCase("difftool")] [TestCase("fetch")] + [TestCase("fmt-merge-msg")] [TestCase("for-each-ref")] + [TestCase("format-patch --stdout --base=auto HEAD~1")] + [TestCase("format-rev HEAD")] + [TestCase("get-tar-commit-id")] + [TestCase("grep pattern")] + [TestCase("grep -in -e pattern -- Readme.md")] + [TestCase("grep -nepattern")] + [TestCase("grep -e -O -f patterns --cached")] + [TestCase("grep -e-O")] + [TestCase("grep --context=2 --threads 1 --color=never pattern")] + [TestCase("grep -- --open-files-in-pager")] + [TestCase("grep -- -O")] [TestCase("help")] [TestCase("hash-object")] [TestCase("index-pack")] + [TestCase("last-modified HEAD -- Readme.md")] [TestCase("log")] [TestCase("ls-files")] [TestCase("ls-remote --symref origin")] [TestCase("ls-remote --get-url origin")] [TestCase("ls-tree")] [TestCase("merge-base")] + [TestCase("merge-file -p ours base theirs")] + [TestCase("merge-file --stdout --diff3 -L ours ours base theirs")] + [TestCase("merge-file -qpLlabel ours base theirs")] + [TestCase("merge-file -p -L --no-stdout ours base theirs")] + [TestCase("merge-file --object-id HEAD:a HEAD:b HEAD:c")] + [TestCase("merge-file --object-id --marker-size=7 --diff-algorithm histogram a b c")] + [TestCase("merge-file --stdout -- ours base --no-stdout")] + [TestCase("merge-tree --write-tree HEAD HEAD")] + [TestCase("mktag")] + [TestCase("mktree")] [TestCase("multi-pack-index")] [TestCase("name-rev")] + [TestCase("notes merge --commit")] [TestCase("pack-objects")] + [TestCase("pack-refs --all")] + [TestCase("patch-id --stable")] + [TestCase("pickaxe HEAD -- Readme.md")] [TestCase("push")] [TestCase("range-diff HEAD~2..HEAD~1 HEAD~1..HEAD")] + [TestCase("reflog")] + [TestCase("reflog show --all")] + [TestCase("reflog list")] + [TestCase("reflog exists HEAD")] + [TestCase("reflog write refs/heads/topic old new message")] + [TestCase("reflog delete --rewrite --updateref topic@{0}")] + [TestCase("reflog drop --all --single-worktree")] + [TestCase("reflog expire --stale-fix --rewrite --updateref --all")] + [TestCase("refs list")] + [TestCase("refs exists refs/heads/topic")] + [TestCase("refs verify --strict")] + [TestCase("refs optimize --all")] + [TestCase("refs create refs/heads/topic HEAD")] + [TestCase("refs delete refs/heads/topic")] + [TestCase("refs update refs/heads/topic HEAD")] + [TestCase("refs rename refs/heads/topic refs/heads/new-topic")] [TestCase("remote")] + [TestCase("replace --edit HEAD")] [TestCase("reset --soft")] [TestCase("rev-list")] [TestCase("rev-parse")] @@ -56,13 +110,18 @@ public class CommandLockingTests [TestCase("show-branch --list")] [TestCase("show-branch --merge-base")] [TestCase("show-branch --reflog")] + [TestCase("show-index")] [TestCase("show-ref")] + [TestCase("stripspace")] [TestCase("submodule")] [TestCase("git-submodule")] [TestCase("symbolic-ref")] [TestCase("tag")] [TestCase("unpack-objects")] [TestCase("update-ref")] + [TestCase("update-server-info")] + [TestCase("url-parse https://example.invalid/repo")] + [TestCase("var GIT_EDITOR")] [TestCase("verify-commit --raw -v HEAD")] [TestCase("verify-pack -v mock-pack.idx")] [TestCase("verify-pack -s mock-pack.idx")] @@ -89,15 +148,46 @@ public void CommandsWithoutLockPreserveTheirPolicy(string command) [TestCase("cherry-pick")] [TestCase("clean")] [TestCase("commit")] - [TestCase("describe")] [TestCase("describe --dirty")] + [TestCase("describe --dirty=modified")] + [TestCase("describe --di")] + [TestCase("describe --dir=modified")] [TestCase("describe --broken")] + [TestCase("describe --br=broken")] + [TestCase("describe --tags --match mock-* --broken")] + [TestCase("describe --unknown-option")] + [TestCase("describe --match")] + [TestCase("describe --git-pid=123")] + [TestCase("grep -O pattern")] + [TestCase("grep -Omock-pager pattern")] + [TestCase("grep -inO pattern")] + [TestCase("grep -nOmock-pager pattern")] + [TestCase("grep --open pattern")] [TestCase("grep --open-files-in-pager=mock-pager pattern")] + [TestCase("grep -e pattern --open-files-in-pager")] + [TestCase("grep --unknown-option pattern")] + [TestCase("grep -e")] + [TestCase("grep --threads")] + [TestCase("grep --exit_code=0 pattern")] [TestCase("merge")] + [TestCase("merge-file ours base theirs")] + [TestCase("merge-file -- -p base theirs")] + [TestCase("merge-file -L -p ours base theirs")] + [TestCase("merge-file -p --no-stdout ours base theirs")] + [TestCase("merge-file --object-id --no-object-id ours base theirs")] + [TestCase("merge-file -p --no-st ours base theirs")] + [TestCase("merge-file --obj ours base theirs")] + [TestCase("merge-file -p --unknown-option ours base theirs")] + [TestCase("merge-file -p -L")] [TestCase("mv")] [TestCase("pull")] [TestCase("read-tree")] [TestCase("rebase")] + [TestCase("refs")] + [TestCase("refs migrate --ref-format=reftable")] + [TestCase("refs unknown list")] + [TestCase("refs --unknown-option list")] + [TestCase("refs LIST")] [TestCase("reset")] [TestCase("reset --mixed")] [TestCase("reset --hard")] @@ -126,6 +216,12 @@ public void CommandsWithLockPreserveTheirPolicy(string command) [TestCase("git-annotate")] [TestCase("git-DIFF")] [TestCase("git-LOG")] + [TestCase("git-reflog")] + [TestCase("git-merge-tree")] + [TestCase("git-describe")] + [TestCase("git-grep pattern")] + [TestCase("git-merge-file -p ours base theirs")] + [TestCase("git-refs list")] [TestCase("git-worktree list")] [TestCase("git-worktree add list")] [TestCase("git-reset --soft")] From 3a6ac09ad83b5a8d9b36a09b8c1dfe4403a9700d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 10 Nov 2026 11:42:47 -0800 Subject: [PATCH 2/2] Preserve hook argument boundaries for partial lock exemptions Quote forwarded arguments with Windows command-line rules. Spaces and embedded quotes must not create false options or hide metadata. Keep partial exemptions locked when the appended metadata is missing. Add installed-loader round trips and quoted-value contention tests. Pin optional-value behavior with adversarial unit cases. Verify each reflog rewrite beside index and tracked-content preservation. Use safe-output names and the shared PID argument prefix. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella --- .../Program.cs | 6 + .../GitReadAndGitLockTests.cs | 137 +++++++++++++++--- GVFS/GVFS.Hooks/Program.CommandLocking.cs | 33 +++-- GVFS/GVFS.Hooks/Program.cs | 1 - .../Hooks/CommandLockingTests.cs | 23 ++- GVFS/GitHooksLoader/GitHooksLoader.cpp | 36 ++++- 6 files changed, 199 insertions(+), 37 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs index b8362179c..083ff033f 100644 --- a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs +++ b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs @@ -8,6 +8,12 @@ public class Program { public static void Main(string[] args) { + if (args.Length > 0 && args[0] == "--echo-arguments") + { + Console.Write(string.Join("\0", args, 1, args.Length - 1)); + return; + } + if (args.Length > 0 && args[0] == "--wait-for-event") { string eventName = Environment.GetEnvironmentVariable("GVFS_TEST_HOOK_WAIT_EVENT_NAME"); diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index 55fa5b06f..815efe121 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -143,17 +143,17 @@ public void ReadOnlyCommandDoesNotWaitForLock(string command) }); } - [TestCase("describe --dirty --always", 0)] - [TestCase("describe --broken --always", 0)] - [TestCase("describe --di --always", 0)] - [TestCase("grep -nOcat -e . -- Readme.md", 0)] - [TestCase("grep --open=cat -e . -- Readme.md", 0)] - [TestCase("refs migrate -h", 0)] - public void UnsafeCommandModeStillWaitsForLock(string command, int expectedExitCode) + [TestCase("describe --dirty --always")] + [TestCase("describe --broken --always")] + [TestCase("describe --di --always")] + [TestCase("grep -nOcat -e . -- Readme.md")] + [TestCase("grep --open=cat -e . -- Readme.md")] + [TestCase("refs migrate -h")] + public void UnsafeCommandModeStillWaitsForLock(string command) { File.ReadAllText(this.Enlistment.GetVirtualPathTo("Readme.md")); ProcessResult result = this.RunWithLockHolder(command); - result.ExitCode.ShouldEqual(expectedExitCode, result.Output + result.Errors); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); result.Errors.ShouldContain(ExpectedStatusWaitingText); } @@ -205,17 +205,17 @@ public void ReflogWritesPreserveIndexAndTrackedContentUnderLockContention() { byte[] index = File.ReadAllBytes(indexPath); DateTime indexWriteTime = File.GetLastWriteTimeUtc(indexPath); - string[] commands = + (string Command, string ExpectedRef, int ReflogEntries)[] commands = { - $"reflog write {refName} {newObject} {newObject} appended", - $"reflog delete --rewrite --updateref {refName}@{{0}}", - $"reflog delete --rewrite --updateref {refName}@{{0}}", - $"reflog expire --stale-fix --rewrite --updateref --expire=never --expire-unreachable=never {refName}", - $"reflog expire --stale-fix --rewrite --updateref --expire=all {refName}", - $"reflog drop {refName}", + ($"reflog write {refName} {newObject} {newObject} appended", newObject, 3), + ($"reflog delete --rewrite --updateref {refName}@{{0}}", newObject, 2), + ($"reflog delete --rewrite --updateref {refName}@{{0}}", oldObject, 1), + ($"reflog expire --stale-fix --rewrite --updateref --expire=never --expire-unreachable=never {refName}", oldObject, 1), + ($"reflog expire --stale-fix --rewrite --updateref --expire=all {refName}", oldObject, 0), + ($"reflog drop {refName}", oldObject, -1), }; - foreach (string command in commands) + foreach ((string command, string expectedRef, int reflogEntries) in commands) { ProcessResult result = GitProcess.InvokeProcess( this.Enlistment.RepoRoot, @@ -223,12 +223,22 @@ public void ReflogWritesPreserveIndexAndTrackedContentUnderLockContention() timeoutMs: AcquireGVFSLockTimeout); result.ExitCode.ShouldEqual(0, result.Output + result.Errors); result.Errors.ShouldNotContain(false, ExpectedStatusWaitingText); + GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse " + refName).Trim().ShouldEqual(expectedRef); + if (reflogEntries < 0) + { + GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "reflog exists " + refName).ExitCode.ShouldEqual(1); + } + else + { + ProcessResult reflog = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "reflog show --format=%H " + refName); + reflog.ExitCode.ShouldEqual(0, reflog.Errors); + reflog.Output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries).Length.ShouldEqual(reflogEntries); + } + File.ReadAllBytes(indexPath).ShouldMatchInOrder(index); File.GetLastWriteTimeUtc(indexPath).ShouldEqual(indexWriteTime); File.ReadAllText(filePath).ShouldEqual(dirtyContent); } - - GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse " + refName).Trim().ShouldEqual(oldObject); }); } finally @@ -238,6 +248,83 @@ public void ReflogWritesPreserveIndexAndTrackedContentUnderLockContention() } } + [TestCase("")] + [TestCase("with space")] + [TestCase("with\ttab")] + [TestCase("with\"quote")] + [TestCase("trailing space slash\\")] + [TestCase("before\\\\\"quote")] + public void HookLoaderPreservesArgumentBoundaries(string argument) + { + string loaderPath = this.Enlistment.GetDotGitPath("hooks", "pre-command.exe"); + string hookListPath = this.Enlistment.GetDotGitPath("hooks", "pre-command.hooks"); + string originalHookList = File.ReadAllText(hookListPath); + try + { + File.WriteAllText(hookListPath, $"\"{GitHelpers.LockHolderCommandPath}\" --echo-arguments"); + ProcessStartInfo loader = new ProcessStartInfo(loaderPath) + { + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + loader.ArgumentList.Add("first"); + loader.ArgumentList.Add(argument); + loader.ArgumentList.Add("last"); + loader.Environment.Remove("GITHOOKSLOADER_PERFTRACE"); + ProcessResult result = ProcessHelper.Run(loader, timeoutMs: AcquireGVFSLockTimeout); + result.ExitCode.ShouldEqual(0, result.Errors); + result.Output.Split('\0').ShouldMatchInOrder("pre-command", "first", argument, "last"); + } + finally + { + File.WriteAllText(hookListPath, originalHookList); + } + } + + [TestCase("rm -f")] + [TestCase("foo --")] + public void GrepOptionValuesCannotHidePagerDispatch(string pattern) + { + File.ReadAllText(this.Enlistment.GetVirtualPathTo("Readme.md")); + string[] arguments = { "grep", "-e", pattern, "-Ocat", "-e", ".", "--", "Readme.md" }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + // Git grep returns 1 when no line matches. + Assert.That(result.ExitCode, Is.InRange(0, 1), result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("v1* --")] + [TestCase("v\"")] + public void DescribeOptionValuesCannotHideIndexRefresh(string pattern) + { + string[] arguments = { "describe", "--match", pattern, "--dirty", "--always" }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("ours -p")] + [TestCase("ours\t-p")] + [TestCase("ours\"")] + public void MergeFileLabelsCannotEnableAnOutputMode(string label) + { + string inputPath = Path.Combine(this.Enlistment.EnlistmentRoot, "merge-label-" + Guid.NewGuid().ToString("N")); + File.WriteAllText(inputPath, "merge input\n"); + try + { + string[] arguments = { "merge-file", "-L", label, inputPath, inputPath, inputPath }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + File.ReadAllText(inputPath).ShouldEqual("merge input\n"); + } + finally + { + File.Delete(inputPath); + } + } + [TestCase, Order(5)] public void GitAliasNamedAfterKnownCommandAcquiresLock() { @@ -677,7 +764,11 @@ private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null, bool shouldWait = true) + private ProcessResult RunWithLockHolder( + string command, + Dictionary environment = null, + bool shouldWait = true, + string[] argumentList = null) { using (Process holder = new Process()) using (Process git = new Process()) @@ -703,6 +794,14 @@ private ProcessResult RunWithLockHolder(string command, Dictionary { if (args.Data != null) diff --git a/GVFS/GVFS.Hooks/Program.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs index 292575c80..5096bea24 100644 --- a/GVFS/GVFS.Hooks/Program.CommandLocking.cs +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -8,11 +8,12 @@ public partial class Program { // Hooks run on the git command path, so config reads retry at most once. private const int ConfigReadMaxAttempts = 2; + private const string GitPidArg = "--git-pid="; // The lock serializes changes to the enlistment's index and tracked working tree with GVFS projection updates. // Before adding an exemption, check all supported options, subcommands, and child-command paths. // Ref and object writes alone do not require this lock. - // Existing diff exemptions allow opportunistic stat-cache refreshes. + // Keep index refresh modes locked. RequiredGitConfig enables opportunistic stat refreshes for diff and difftool. // Keep arbitrary Git dispatchers locked because nested command hooks can be suppressed. // Excluded readers can observe concurrent working-tree changes. They do not receive snapshot isolation. internal static bool TryShouldLock( @@ -134,7 +135,7 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) return !HasOnlySafeOptions(args, GetGrepOptionType, out _); case "merge-file": - return !HasOnlySafeOptions(args, GetMergeFileOptionType, out bool hasReadOnlyMode) || !hasReadOnlyMode; + return !HasOnlySafeOptions(args, GetMergeFileOptionType, out bool hasSafeOutputMode) || !hasSafeOutputMode; case "refs": switch (args.Length > 2 ? args[2] : null) @@ -176,22 +177,28 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) return true; } - private static bool HasOnlySafeOptions(string[] args, Func getOptionType, out bool hasReadOnlyMode) + private static bool HasOnlySafeOptions(string[] args, Func getOptionType, out bool hasSafeOutputMode) { - hasReadOnlyMode = false; + hasSafeOutputMode = false; int end = args.Length; // Only the appended hook metadata is excluded. User arguments with these names remain unknown options. - if (end > 2 && args[0] == "post-command" && args[end - 1].StartsWith("--exit_code=", StringComparison.Ordinal)) + if (args[0] == "post-command") { + if (end <= 2 || !args[end - 1].StartsWith("--exit_code=", StringComparison.Ordinal)) + { + return false; + } + --end; } - if (end > 2 && args[end - 1].StartsWith("--git-pid=", StringComparison.Ordinal)) + if (end <= 2 || !args[end - 1].StartsWith(GitPidArg, StringComparison.Ordinal)) { - --end; + return false; } + --end; for (int i = 2; i < end; ++i) { string argument = args[i]; @@ -216,7 +223,7 @@ private static bool HasOnlySafeOptions(string[] args, Func= end) { return false; @@ -232,10 +239,10 @@ private static bool HasOnlySafeOptions(string[] args, Func= end) + if (j == argument.Length - 1 && ++i >= end) { return false; } @@ -363,7 +370,7 @@ private static CommandOptionType GetMergeFileOptionType(string option) case "-p": case "--stdout": case "--object-id": - return CommandOptionType.ReadOnlyMode; + return CommandOptionType.SafeOutputMode; case "-q": case "--quiet": @@ -400,7 +407,7 @@ private enum CommandOptionType Flag, Value, OptionalValue, - ReadOnlyMode, + SafeOutputMode, } } } diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 600ee823a..4c51a0c63 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -17,7 +17,6 @@ public partial class Program private const string PreCommandHook = "pre-command"; private const string PostCommandHook = "post-command"; - private const string GitPidArg = "--git-pid="; private const int InvalidProcessId = -1; private const int PostCommandSpinnerDelayMs = 500; diff --git a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs index 1fe855a7b..40ab5c07f 100644 --- a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs @@ -31,6 +31,8 @@ public class CommandLockingTests [TestCase("credential")] [TestCase("describe")] [TestCase("describe --tags --always --abbrev=7 HEAD")] + [TestCase("describe --abbrev")] + [TestCase("describe --abbrev 7")] [TestCase("describe --contains --all --long --first-parent --exact-match")] [TestCase("describe --match mock-* --exclude=excluded-* --candidates 5")] [TestCase("describe --match --dirty")] @@ -53,6 +55,7 @@ public class CommandLockingTests [TestCase("grep -e -O -f patterns --cached")] [TestCase("grep -e-O")] [TestCase("grep --context=2 --threads 1 --color=never pattern")] + [TestCase("grep --color pattern")] [TestCase("grep -- --open-files-in-pager")] [TestCase("grep -- -O")] [TestCase("help")] @@ -150,6 +153,7 @@ public void CommandsWithoutLockPreserveTheirPolicy(string command) [TestCase("commit")] [TestCase("describe --dirty")] [TestCase("describe --dirty=modified")] + [TestCase("describe --abbrev --dirty")] [TestCase("describe --di")] [TestCase("describe --dir=modified")] [TestCase("describe --broken")] @@ -159,6 +163,8 @@ public void CommandsWithoutLockPreserveTheirPolicy(string command) [TestCase("describe --match")] [TestCase("describe --git-pid=123")] [TestCase("grep -O pattern")] + [TestCase("grep --color -O pattern")] + [TestCase("grep --cached=value pattern")] [TestCase("grep -Omock-pager pattern")] [TestCase("grep -inO pattern")] [TestCase("grep -nOmock-pager pattern")] @@ -213,6 +219,17 @@ public void CommandsWithLockPreserveTheirPolicy(string command) AssertLockPolicy(command, shouldLock: true); } + [TestCase("describe")] + [TestCase("describe --match value")] + [TestCase("grep pattern")] + [TestCase("grep --color pattern")] + [TestCase("merge-file -p a b c")] + [TestCase("merge-file --object-id a b c")] + public void PartialExemptionsRequireSeparateHookMetadata(string command) + { + AssertLockPolicy(command, shouldLock: true, includeHookMetadata: false); + } + [TestCase("git-annotate")] [TestCase("git-DIFF")] [TestCase("git-LOG")] @@ -290,7 +307,7 @@ private static string[] CreatePreCommandArgs(string command) return new[] { "pre-command" }.Concat(command.Split(' ')).Concat(new[] { "--git-pid=456" }).ToArray(); } - private static void AssertLockPolicy(string command, bool shouldLock) + private static void AssertLockPolicy(string command, bool shouldLock, bool includeHookMetadata = true) { // The hook sends post-command for git- to the lock-ownership check, not to this policy. string[] hooks = command.StartsWith("git-", System.StringComparison.Ordinal) @@ -300,7 +317,9 @@ private static void AssertLockPolicy(string command, bool shouldLock) { string[] args = new[] { hook } .Concat(command.Split(' ')) - .Concat(hook == "post-command" + .Concat(!includeHookMetadata + ? System.Array.Empty() + : hook == "post-command" ? new[] { "--git-pid=456", "--exit_code=0" } : new[] { "--git-pid=456" }) .ToArray(); diff --git a/GVFS/GitHooksLoader/GitHooksLoader.cpp b/GVFS/GitHooksLoader/GitHooksLoader.cpp index 9f1619d0a..e9ddc6ed8 100644 --- a/GVFS/GitHooksLoader/GitHooksLoader.cpp +++ b/GVFS/GitHooksLoader/GitHooksLoader.cpp @@ -2,6 +2,7 @@ // #include "stdafx.h" +#include #include #include @@ -91,6 +92,36 @@ int wmain(int argc, WCHAR *argv[]) return 0; } +static void AppendCommandLineArgument(std::wstring &commandLine, const wchar_t *argument) +{ + commandLine += L' '; + if (*argument != L'\0' && std::wcspbrk(argument, L" \t\"") == nullptr) + { + commandLine += argument; + return; + } + + commandLine += L'"'; + size_t backslashes = 0; + for (; *argument != L'\0'; ++argument) + { + if (*argument == L'\\') + { + ++backslashes; + } + else + { + // Double backslashes before quotes, then escape the literal quote. + commandLine.append(*argument == L'"' ? backslashes * 2 + 1 : backslashes, L'\\'); + commandLine += *argument; + backslashes = 0; + } + } + + commandLine.append(backslashes * 2, L'\\'); + commandLine += L'"'; +} + int ExecuteHook(const std::wstring &applicationName, wchar_t *hookName, int argc, WCHAR *argv[]) { wchar_t expandedPath[MAX_PATH + 1]; @@ -101,10 +132,11 @@ int ExecuteHook(const std::wstring &applicationName, wchar_t *hookName, int argc exit(6); } - std::wstring commandLine = std::wstring(expandedPath) + L" " + hookName; + std::wstring commandLine = std::wstring(expandedPath); + AppendCommandLineArgument(commandLine, hookName); for (int x = 1; x < argc; x++) { - commandLine += L" " + std::wstring(argv[x]); + AppendCommandLineArgument(commandLine, argv[x]); } // Start the child process.