Fix/pinvoke review feedback

LibLouis.NET.Test/LiblouisHeapTests.cs

@@ -0,0 +1,153 @@
+using System;
+using System.IO;
+using System.Linq;
+using System.Runtime.InteropServices;
+using System.Text;
+
+using Xunit;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// lou_findTable's result is the caller's to free, with liblouis's own C runtime. On Windows that
+/// is found through liblouis's import table, so the table walk is tested here against a synthetic
+/// image on every platform, and against the real liblouis.dll on Windows.
+/// </summary>
+public unsafe class LiblouisHeapTests
+{
+    private static readonly string[] Tables = ["da-dk-g26.ctb", "da-dk-g16-markers.ctb"];
+
+    private static string[] TablePaths() =>
+        [.. Tables.Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))];
+
+    // RVAs in the synthetic image.
+    private const int ImportTable = 0x200;
+    private const int NameThunks = 0x300;
+    private const int AddressThunks = 0x400;
+    private const int MallocName = 0x500;
+    private const int FreeName = 0x520;
+    private const int LibraryName = 0x540;
+
+    /// <summary>
+    /// A minimal mapped PE image importing, from one library: an ordinal, malloc and free, bound to
+    /// 0x1111, 0x2222 and 0x3333.
+    /// </summary>
+    private static byte[] SyntheticImage()
+    {
+        byte[] image = new byte[0x600];
+        Span<byte> span = image;
+
+        BitConverter.TryWriteBytes(span[0x00..], (ushort)0x5A4D);    // "MZ"
+        BitConverter.TryWriteBytes(span[0x3C..], 0x40);              // e_lfanew
+        BitConverter.TryWriteBytes(span[0x40..], 0x00004550u);       // "PE\0\0"
+
+        // Optional header at 0x40 + 24; the import directory is data directory 1.
+        int dataDirectories = 0x40 + 24 + (IntPtr.Size == 8 ? 112 : 96);
+        BitConverter.TryWriteBytes(span[(dataDirectories + 8)..], ImportTable);
+
+        // One descriptor, then the all-zero terminator.
+        BitConverter.TryWriteBytes(span[(ImportTable + 0)..], NameThunks);
+        BitConverter.TryWriteBytes(span[(ImportTable + 12)..], LibraryName);
+        BitConverter.TryWriteBytes(span[(ImportTable + 16)..], AddressThunks);
+
+        nint ordinal = IntPtr.Size == 8 ? unchecked((nint)0x8000_0000_0000_0007) : unchecked((nint)0x8000_0007);
+        WriteThunks(span[NameThunks..], [ordinal, MallocName, FreeName, 0]);
+        WriteThunks(span[AddressThunks..], [0x1111, 0x2222, 0x3333, 0]);
+
+        // IMAGE_IMPORT_BY_NAME: a two byte hint, then the name.
+        Encoding.ASCII.GetBytes("malloc\0").CopyTo(span[(MallocName + 2)..]);
+        Encoding.ASCII.GetBytes("free\0").CopyTo(span[(FreeName + 2)..]);
+        Encoding.ASCII.GetBytes("msvcrt.dll\0").CopyTo(span[LibraryName..]);
+
+        return image;
+    }
+
+    private static void WriteThunks(Span<byte> destination, nint[] thunks)
+    {
+        MemoryMarshal.AsBytes(thunks.AsSpan()).CopyTo(destination);
+    }
+
+    [Theory]
+    [InlineData("free", 0x3333)]
+    [InlineData("malloc", 0x2222)]
+    [InlineData("realloc", 0)]
+    [InlineData("fre", 0)]
+    public void FindImport_ReturnsTheBoundAddressOfANamedImport(string name, int expected)
+    {
+        fixed (byte* image = SyntheticImage())
+        {
+            Assert.Equal(expected, (int)LiblouisHeap.FindImport(image, Encoding.ASCII.GetBytes(name)));
+        }
+    }
+
+    /// <summary>
+    /// Without OriginalFirstThunk the names have been overwritten by addresses, so the descriptor
+    /// cannot be searched - it must be skipped rather than read as names.
+    /// </summary>
+    [Fact]
+    public void FindImport_SkipsADescriptorWithoutNames()
+    {
+        byte[] bytes = SyntheticImage();
+        BitConverter.TryWriteBytes(bytes.AsSpan(ImportTable), 0);
+
+        fixed (byte* image = bytes)
+        {
+            Assert.Equal(0, (int)LiblouisHeap.FindImport(image, "free"u8));
+        }
+    }
+
+    [Fact]
+    public void FindImport_RejectsSomethingThatIsNotAPeImage()
+    {
+        fixed (byte* image = new byte[0x600])
+        {
+            Assert.Equal(0, (int)LiblouisHeap.FindImport(image, "free"u8));
+        }
+    }
+
+    /// <summary>
+    /// The real liblouis.dll has to import free() from a C runtime, or every lou_findTable result
+    /// would be left unfreed.
+    /// </summary>
+    [Fact]
+    public void FindImport_FindsFreeInTheLiblouisWeShip()
+    {
+        if (!OperatingSystem.IsWindows())
+        {
+            return;
+        }
+
+        // Make sure the P/Invoke layer has loaded it.
+        _ = LibLouis.Instance.Version;
+
+        nint module = NativeLibrary.Load("liblouis", typeof(LibLouis).Assembly, DllImportSearchPath.SafeDirectories);
+        nint free = LiblouisHeap.FindImport((byte*)module, "free"u8);
+
+        nint[] runtimeFrees = [.. new[] { "msvcrt.dll", "ucrtbase.dll" }
+            .Select(name => NativeLibrary.TryLoad(name, out nint runtime) ? NativeLibrary.GetExport(runtime, "free") : 0)];
+
+        Assert.NotEqual(0, free);
+        Assert.Contains(free, runtimeFrees);
+    }
+
+    /// <summary>
+    /// Each result is freed into liblouis's heap as it comes back. Releasing it into a different
+    /// heap corrupts the one liblouis translates with, so interleave the two and repeat enough for
+    /// the corruption to surface as a crash.
+    /// </summary>
+    [Fact]
+    public void FindTable_ResultsCanBeFreedBetweenTranslations()
+    {
+        LibLouis.Instance.IndexTables(TablePaths());
+
+        for (int k = 0; k < 2000; k++)
+        {
+            string? table = LibLouis.Instance.FindTable("language:da type:literary grade:2");
+
+            Assert.NotNull(table);
+            Assert.EndsWith("da-dk-g26.ctb", table, StringComparison.Ordinal);
+
+            Assert.NotEmpty(LibLouis.Instance.Translate(TablePaths(), "Første linje", 64, null, null, TranslationMode.Regular));
+        }
+    }
+}

LibLouis.NET.Test/NativeLockTests.cs

@@ -180,7 +180,7 @@ private static string ReadLibrarySource(string fileName)
     {
         DirectoryInfo? directory = new(AppContext.BaseDirectory);
 
-        while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "LibLouis.NET.sln")))
+        while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "LibLouis.NET.slnx")))
         {
             directory = directory.Parent;
         }

LibLouis.NET.Test/PositionMappingTests.cs

@@ -178,4 +178,77 @@ public void CursorPositionIsAnIndexIntoTheOutput()
         Assert.InRange(result.CursorPosition, 0, result.Output.Length - 1);
         Assert.False(char.IsLowSurrogate(result.Output[result.CursorPosition]));
     }
+
+    /// <summary>
+    /// A cursor after the last character - where it sits while appending - has to stay after the
+    /// last character, not move back onto it.
+    /// </summary>
+    [Theory]
+    [InlineData("abc")]
+    [InlineData("a\U0001D11E")]
+    [InlineData("")]
+    public void CursorAtEndOfInputStaysAtEndOfOutput(string input)
+    {
+        int outputLength = Math.Max(16, input.Length * 4);
+
+        TranslatedString result = LibLouis.Instance.Translate(
+            TablePaths(),
+            input,
+            outputLength,
+            null,
+            null,
+            new int[input.Length],
+            new int[outputLength],
+            input.Length,
+            TranslationMode.Regular);
+
+        Assert.Equal(result.Output.Length, result.CursorPosition);
+    }
+
+    /// <summary>
+    /// The same going the other way.
+    /// </summary>
+    [Fact]
+    public void BackTranslateCursorAtEndOfInputStaysAtEndOfOutput()
+    {
+        string braille = Translate("abc").Output;
+        int outputLength = braille.Length * 4;
+
+        TranslatedString result = LibLouis.Instance.BackTranslate(
+            TablePaths(),
+            braille,
+            outputLength,
+            null,
+            null,
+            new int[braille.Length],
+            new int[outputLength],
+            braille.Length,
+            TranslationMode.Regular);
+
+        Assert.Equal(result.Output.Length, result.CursorPosition);
+    }
+
+    /// <summary>
+    /// The last character is still a character: only the position after it is the end.
+    /// </summary>
+    [Fact]
+    public void CursorOnLastCharacterStaysOnIt()
+    {
+        const string input = "abc";
+
+        int outputLength = input.Length * 4;
+
+        TranslatedString result = LibLouis.Instance.Translate(
+            TablePaths(),
+            input,
+            outputLength,
+            null,
+            null,
+            new int[input.Length],
+            new int[outputLength],
+            input.Length - 1,
+            TranslationMode.Regular);
+
+        Assert.Equal(result.OutputPosition[^1], result.CursorPosition);
+    }
 }

LibLouis.NET.Test/ReturnedStringOwnershipTests.cs

@@ -6,15 +6,13 @@
 namespace LibLouis.NET.Test;
 
 /// <summary>
-/// liblouis owns every string it returns, so the wrapper must not hand those pointers to the
-/// marshaller's Free.
+/// The strings liblouis returns must not be freed by the default marshaller.
 ///
 ///   * lou_setDataPath / lou_getDataPath return a pointer into a static char[MAXSTRING] inside
 ///     liblouis (compileTranslationTable.c:59-73). Passing that to free() is undefined behaviour
 ///     on every platform.
-///   * lou_findTable returns malloc'd memory. Our Windows binaries are built with mingw-w64 and
-///     allocate from msvcrt.dll, while .NET frees through ucrtbase.dll - different heaps, so
-///     freeing it from managed code corrupts the heap there.
+///   * lou_findTable returns malloc'd memory the caller frees, but with liblouis's C runtime
+///     rather than the CoTaskMemFree .NET would use on Windows - see LiblouisHeapTests.
 /// </summary>
 public class ReturnedStringOwnershipTests
 {

LibLouis.NET/LibLouis.cs

@@ -367,7 +367,8 @@ public TranslatedString Translate(
         byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength);
         TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength);
 
-        // The cursor arrives as a .NET string index and liblouis wants a widechar index.
+        // The cursor arrives as a .NET string index and liblouis wants a widechar index. A cursor
+        // at the end of the input is not passed on, but placed at the end of the output below.
         int[] inputOffsets = Utf16OffsetOfWidechar(input);
         int widecharCursor = ToWidecharCursor(input, cursorPosition);
 
@@ -393,7 +394,7 @@ public TranslatedString Translate(
         return new TranslatedString
         {
             Output = output,
-            CursorPosition = mappedCursor,
+            CursorPosition = IsCursorAtEnd(input, cursorPosition) ? output.Length : mappedCursor,
             InputPosition = mappedInputPosition,
             OutputPosition = mappedOutputPosition,
             OutputDots78 = ExtractOutputDots78(typeFormBuffer, outputLength),
@@ -512,7 +513,8 @@ public TranslatedString BackTranslate(
         byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength);
         TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength);
 
-        // The cursor arrives as a .NET string index and liblouis wants a widechar index.
+        // The cursor arrives as a .NET string index and liblouis wants a widechar index. A cursor
+        // at the end of the input is not passed on, but placed at the end of the output below.
         int[] inputOffsets = Utf16OffsetOfWidechar(input);
         int widecharCursor = ToWidecharCursor(input, cursorPosition);
 
@@ -538,7 +540,7 @@ public TranslatedString BackTranslate(
         return new TranslatedString
         {
             Output = output,
-            CursorPosition = mappedCursor,
+            CursorPosition = IsCursorAtEnd(input, cursorPosition) ? output.Length : mappedCursor,
             InputPosition = mappedInputPosition,
             OutputPosition = mappedOutputPosition,
         };
@@ -708,22 +710,31 @@ public string Hyphenate(IEnumerable<string> tableList, string input, Translation
         return dots;
     }
 
+    /// <summary>
+    /// Whether the cursor sits at the end of <paramref name="input"/> - after its last character,
+    /// where a caller appending text puts it.
+    /// </summary>
+    private static bool IsCursorAtEnd(string input, int cursorPosition) => cursorPosition >= input.Length;
+
     /// <summary>
     /// Converts a cursor given as a .NET string index into the widechar index liblouis expects.
     /// </summary>
     /// <remarks>
-    /// Negative means "no cursor" to liblouis and is passed through untouched.
+    /// Returns -1, "no cursor" to liblouis, for a negative cursor and for one at the end of the
+    /// input. liblouis reports the cursor back by reading <c>outputPos[*cursorPos]</c>
+    /// (lou_translateString.c:1389, lou_backTranslateString.c:350), which is only in bounds for a
+    /// character of the input: at the end it reads one past the array, and for anything negative
+    /// but -1 it reads before it. The caller places an end cursor itself, see
+    /// <see cref="IsCursorAtEnd"/>.
     /// </remarks>
     private int ToWidecharCursor(string input, int cursorPosition)
     {
-        if (cursorPosition < 0 || input.Length == 0)
+        if (cursorPosition < 0 || IsCursorAtEnd(input, cursorPosition))
         {
-            return cursorPosition;
+            return -1;
         }
 
-        int[] widechars = WidecharOfUtf16Offset(input);
-
-        return widechars[Math.Clamp(cursorPosition, 0, input.Length - 1)];
+        return WidecharOfUtf16Offset(input)[cursorPosition];
     }
 
     /// <summary>
@@ -779,7 +790,7 @@ private int ToWidecharCursor(string input, int cursorPosition)
             inputPosition[t] = inputOffsets[Math.Clamp(character, 0, lastInputWidechar)];
         }
 
-        // A negative cursor means "no cursor" to liblouis; leave it alone.
+        // -1 means "no cursor" to liblouis; leave it alone.
         int cursorPosition = widecharCursor < 0 || output.Length == 0
             ? widecharCursor
             : outputOffsets[Math.Clamp(widecharCursor, 0, lastOutputWidechar)];