diff --git a/AGENTS.md b/AGENTS.md index e5dd769f3..2698dd58f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,6 +199,37 @@ Default new gates to `false` and gate the **runtime entry point** into a feature, not its build, so the code still compiles and ships (and keeps getting exercised) while its behavior stays off by default. +## libgit2 P/Invoke string marshalling (UTF-8, not ANSI) + +The libgit2 bindings live in `GVFS.Common/Git/LibGit2Repo.cs` under the +`Native` class. **libgit2 treats every string it receives and returns as +UTF-8** — paths, revspecs, config keys and values, and error messages. + +The .NET default for a bare `[DllImport]` string parameter is +`CharSet.Ansi`, which encodes through the Windows ANSI code page and +**silently corrupts non-ASCII input** (a repo path under a non-English user +name, a non-ASCII branch name, etc.) — an unmappable character becomes `?`, +so `git_repository_open` and friends resolve the wrong path or fail. There is +no `[module: DefaultCharSet]` override in `GVFS.Common`, so the ANSI default +applies unless each declaration opts out. + +When you add a libgit2 P/Invoke: + +- Annotate **every** `string` parameter, `out string` parameter, string return + value, and string struct field with + `[MarshalAs(UnmanagedType.LPUTF8Str)]`. +- Do **not** use `CharSet.Unicode` — that marshals UTF-16, which libgit2 does + not accept (it is the same bug in the other direction). +- For a function that returns a **borrowed** pointer owned by libgit2 (e.g. + `git_config_get_string`), marshal it as `IntPtr` and copy with + `Marshal.PtrToStringUTF8(...)`; do not marshal it as `out string`, or the + interop marshaller frees libgit2's heap memory with the wrong allocator. + +Because the mismatch only manifests through the native call, cover new +string-carrying paths with a real-libgit2 test that uses a non-ASCII input +(see `GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs`), not a +mock-based unit test. + ## Coding standards See [CONTRIBUTING.md](CONTRIBUTING.md) for StyleCop rules, error-handling diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index b21eb83b9..6667dd2c8 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -7,6 +7,24 @@ namespace GVFS.Common.Git { + /// + /// In-process wrapper over libgit2 (git2.dll) for local Git object, tree, blob, and + /// config lookups without spawning git.exe. + /// + /// + /// libgit2 marshalling contract: libgit2 treats EVERY string it receives and returns as + /// UTF-8 (paths, revspecs, config keys and values, error messages). The default marshalling + /// for a bare [DllImport] string parameter is , which encodes + /// through the Windows ANSI code page and silently corrupts (or drops, as '?') any non-ASCII + /// byte — for example a repository path under a non-English Windows user name. When adding a + /// new libgit2 P/Invoke to , annotate every string parameter, + /// out string parameter, string return value, and string struct field with + /// [MarshalAs(UnmanagedType.LPUTF8Str)]. Do NOT use — that + /// marshals UTF-16, which libgit2 does not accept. For a libgit2 function that returns a borrowed + /// pointer owned by libgit2 (for example git_config_get_string), marshal it as + /// and copy with so the + /// interop marshaller does not free libgit2's heap memory. + /// public class LibGit2Repo : IDisposable { private bool disposedValue = false; @@ -572,6 +590,12 @@ protected bool CheckSafeDirectoryConfigForCaseSensitivityIssue(ITracer tracer, s return configuredMatchingDirectory != null && TryOpenRepo(configuredMatchingDirectory, out repoHandle) == Native.ResultCode.Success; } + /// + /// Raw libgit2 P/Invoke declarations. See the marshalling contract on + /// : every string parameter/return/field passed to or from + /// libgit2 MUST be [MarshalAs(UnmanagedType.LPUTF8Str)] (never the ANSI default, + /// never ). + /// public static class Native { public enum ResultCode : int @@ -602,7 +626,7 @@ public static GitOid IntPtrToGitOid(IntPtr oidPtr) public static extern int Shutdown(); [DllImport(Git2NativeLibName, EntryPoint = "git_revparse_single")] - public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, string oid); + public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string oid); public static string GetLastError() { @@ -612,7 +636,11 @@ public static string GetLastError() return "Operation was successful"; } - return Marshal.PtrToStructure(ptr).Message; + // git_error.message is a borrowed UTF-8 char* owned by libgit2. Keep it as an + // IntPtr and copy it with Marshal.PtrToStringUTF8 so the interop marshaller never + // frees libgit2's heap memory (same pattern as GitConfigEntry). + GitError error = Marshal.PtrToStructure(ptr); + return error.Message == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(error.Message); } [DllImport(Git2NativeLibName, EntryPoint = "git_error_last")] @@ -621,8 +649,7 @@ public static string GetLastError() [StructLayout(LayoutKind.Sequential)] private struct GitError { - [MarshalAs(UnmanagedType.LPStr)] - public string Message; + public IntPtr Message; public int Klass; } @@ -630,7 +657,7 @@ private struct GitError public static class Repo { [DllImport(Git2NativeLibName, EntryPoint = "git_repository_open")] - public static extern ResultCode Open(out IntPtr repoHandle, string path); + public static extern ResultCode Open(out IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string path); [DllImport(Git2NativeLibName, EntryPoint = "git_repository_free")] public static extern void Free(IntPtr repoHandle); @@ -649,7 +676,7 @@ public static class Odb public static extern void Free(IntPtr odbHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_oid_fromstr")] - public static extern ResultCode OidFromStr(out GitOid oid, string str); + public static extern ResultCode OidFromStr(out GitOid oid, [MarshalAs(UnmanagedType.LPUTF8Str)] string str); } public static class Config @@ -664,13 +691,13 @@ public static class Config public static extern ResultCode Snapshot(out IntPtr snapshotConfigHandle, IntPtr configHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_string")] - public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, string name); + public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_multivar_foreach")] public static extern ResultCode GetMultivarForeach( IntPtr configHandle, - string name, - string regex, + [MarshalAs(UnmanagedType.LPUTF8Str)] string name, + [MarshalAs(UnmanagedType.LPUTF8Str)] string regex, GitConfigMultivarCallback callback, IntPtr payload); @@ -719,7 +746,7 @@ private static string MarshalUtf8String(IntPtr ptr) } [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_bool")] - public static extern ResultCode GetBool(out bool value, IntPtr configHandle, string name); + public static extern ResultCode GetBool(out bool value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_free")] public static extern void Free(IntPtr configHandle); diff --git a/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs new file mode 100644 index 000000000..d641ac677 --- /dev/null +++ b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs @@ -0,0 +1,107 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.IO; +using System.Runtime.InteropServices; +using System.Text; +using GitProcess = GVFS.FunctionalTests.Tools.GitProcess; + +namespace GVFS.FunctionalTests.Tests +{ + /// + /// Exercises the real libgit2 (git2.dll) open/read path in against a + /// repository whose path and config value contain non-ASCII characters. This is a regression + /// guard for the string-marshalling bug where the libgit2 P/Invoke declarations used the + /// implicit default instead of UTF-8: + /// on a non-UTF-8 Windows code page the ANSI marshaller drops an unmappable character to '?', + /// so git_repository_open resolves the wrong path and the constructor throws. No + /// mock-based unit test can catch this because it only manifests through the native P/Invoke. + /// + [TestFixture] + public class LibGit2NonAsciiPathTests + { + // "café_日本語_☃" — Latin-1 accent, CJK, and a symbol outside every common Windows ANSI + // code page. Built from \u escapes so the reproduction does not depend on this source + // file's on-disk encoding. + private const string NonAsciiToken = "caf\u00e9_\u65e5\u672c\u8a9e_\u2603"; + + private const string StringConfigKey = "gvfs.functionaltests-nonascii"; + private const string NonAsciiConfigValue = "caf\u00e9-\u65e5\u672c\u8a9e-\u2603"; + + private string repoRoot; + + [OneTimeSetUp] + public void CreateRepo() + { + // The ANSI default marshalling only corrupts non-ASCII characters when the active + // Windows ANSI code page cannot represent them. If the process ANSI code page is + // UTF-8 (CP65001, the "Beta: Use Unicode UTF-8" setting), ANSI marshalling already + // produces correct UTF-8, so the bug cannot reproduce and this guard is inconclusive. + // Log the active code page so a reader of the test results can tell whether this guard + // actually ran (e.g. CP1252 on the standard CI runners) or was skipped as inconclusive. + const uint CP_UTF8 = 65001; + uint activeCodePage = GetACP(); + TestContext.Progress.WriteLine($"{nameof(LibGit2NonAsciiPathTests)}: active ANSI code page = {activeCodePage}"); + if (activeCodePage == CP_UTF8) + { + Assert.Ignore( + $"Active ANSI code page is UTF-8 (CP{CP_UTF8}); ANSI marshalling already produces UTF-8, so the " + + "ANSI-vs-UTF-8 marshalling bug cannot reproduce and this regression guard is inconclusive here. " + + "Run on a non-UTF-8 code page (e.g. CP1252, the standard CI runner default) to exercise it."); + } + + this.repoRoot = Path.Combine( + Path.GetTempPath(), + "GVFS.LibGit2NonAsciiPathTests_" + NonAsciiToken + "_" + Path.GetRandomFileName()); + Directory.CreateDirectory(this.repoRoot); + + GitProcess.Invoke(this.repoRoot, "init"); + GitProcess.Invoke(this.repoRoot, "config user.name \"Functional Test User\""); + GitProcess.Invoke(this.repoRoot, "config user.email \"functional@test.com\""); + + // Write the non-ASCII config value straight to .git/config as UTF-8 (no BOM) so the + // value round-tripped through libgit2 is not confounded by how git.exe would encode a + // command-line argument. + string configPath = Path.Combine(this.repoRoot, ".git", "config"); + File.AppendAllText( + configPath, + $"[gvfs]\n\tfunctionaltests-nonascii = {NonAsciiConfigValue}\n", + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)); + } + + [OneTimeTearDown] + public void DeleteRepo() + { + if (this.repoRoot != null) + { + RepositoryHelpers.DeleteTestDirectory(this.repoRoot); + } + } + + [TestCase] + public void OpensRepositoryAtNonAsciiPath() + { + // Before the LPUTF8Str marshalling fix, git_repository_open received an ANSI-mangled + // path, failed, and the constructor threw InvalidDataException. + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + // Reading a known config value proves the RepoHandle is real, not just non-null. + repo.GetConfigString("user.email").ShouldEqual("functional@test.com"); + } + } + + [TestCase] + public void GetConfigStringReturnsNonAsciiValue() + { + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigString(StringConfigKey).ShouldEqual(NonAsciiConfigValue); + } + } + + [DllImport("kernel32.dll")] + private static extern uint GetACP(); + } +}