From b4e91944687ce8ab60bf6b590b4f0c9de3f06039 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 21 Sep 2026 16:01:32 -0700 Subject: [PATCH] Marshal libgit2 P/Invoke strings as UTF-8, not ANSI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libgit2 treats every string it receives and returns as UTF-8. The LibGit2Repo P/Invoke declarations left CharSet unset, so a bare [DllImport] string parameter defaulted to CharSet.Ansi and encoded through the Windows ANSI code page. On a non-UTF-8 code page an unmappable character (for example a repository path under a non-English Windows user name, such as "C:\Users\...\café_日本語") was dropped to '?', so git_repository_open resolved the wrong path and the constructor threw InvalidDataException. Annotate every string parameter with [MarshalAs(UnmanagedType.LPUTF8Str)]. This covers the repository path (git_repository_open), the revspec (git_revparse_single), the object id string (git_oid_fromstr), and the config keys (git_config_get_string, git_config_get_bool, git_config_get_multivar_foreach). Read the borrowed git_error.message pointer as an IntPtr and copy it with Marshal.PtrToStringUTF8, matching the existing GitConfigEntry pattern, so the interop marshaller never frees libgit2's heap memory. The config value output path already used Marshal.PtrToStringUTF8, so it is unchanged. Add LibGit2NonAsciiPathTests, which opens a real repository at a non-ASCII path through git2.dll and reads a non-ASCII config value. Before the fix both cases fail with "No mapping for the Unicode character exists in the target multi-byte code page"; after the fix they pass. The fixture ignores itself when the active ANSI code page is already UTF-8 (CP65001), where the bug cannot reproduce. Document the UTF-8 marshalling contract on the LibGit2Repo class and in AGENTS.md so future libgit2 bindings do not regress. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- AGENTS.md | 31 +++++ GVFS/GVFS.Common/Git/LibGit2Repo.cs | 47 ++++++-- .../Tests/LibGit2NonAsciiPathTests.cs | 107 ++++++++++++++++++ 3 files changed, 175 insertions(+), 10 deletions(-) create mode 100644 GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs diff --git a/AGENTS.md b/AGENTS.md index e5dd769f3d..2698dd58f7 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 b21eb83b9e..6667dd2c82 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 0000000000..d641ac6770 --- /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(); + } +}