Memory, thread and lifecycle safety in the P/Invoke layer

LibLouis.NET.Test/AssemblyInfo.cs

@@ -0,0 +1,7 @@
+using Xunit;
+
+// liblouis keeps global state (compiled table cache, log callback, data path) and is explicitly
+// not thread safe - LibLouis serialises its own calls behind a lock for exactly that reason.
+// xunit parallelises across test classes by default, which lets unsynchronised native calls race
+// and produce spurious "could not be compiled" failures. Run the whole assembly serially.
+[assembly: CollectionBehavior(DisableTestParallelization = true)]

LibLouis.NET.Test/CollectingLogger.cs

@@ -0,0 +1,30 @@
+using System;
+using System.Collections.Generic;
+
+using Microsoft.Extensions.Logging;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// Captures everything liblouis logs, so tests can assert on what the native side reported.
+/// </summary>
+internal sealed class CollectingLogger : ILogger
+{
+    public List<string> Messages { get; } = [];
+
+    public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+
+    // LogLevel is qualified throughout: in this namespace the unqualified name binds to
+    // LibLouis.NET.LogLevel, the native enum, not the Microsoft.Extensions.Logging one.
+    public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true;
+
+    public void Log<TState>(
+        Microsoft.Extensions.Logging.LogLevel logLevel,
+        EventId eventId,
+        TState state,
+        Exception? exception,
+        Func<TState, Exception?, string> formatter)
+    {
+        Messages.Add(formatter(state, exception));
+    }
+}

LibLouis.NET.Test/HyphenateTests.cs

@@ -0,0 +1,70 @@
+using System;
+using System.IO;
+using System.Linq;
+using System.Text.RegularExpressions;
+
+using Xunit;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// lou_hyphenate takes a caller-allocated <c>char *hyphens</c> buffer and writes inlen + 1 bytes
+/// into it: '0' or '1' per character, plus a terminator (lou_translateString.c:4080).
+/// </summary>
+public class HyphenateTests
+{
+    private const string Word = "bogstaver";
+
+    private static readonly string[] Tables = ["da-dk-braillo.dis", "da-dk-g26.ctb"];
+
+    private static string[] TablePaths() =>
+        [.. Tables.Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))];
+
+    [Fact]
+    public void Hyphenate_ReturnsOneHyphenationFlagPerCharacter()
+    {
+        string hyphens = LibLouis.Instance.Hyphenate(TablePaths(), Word, TranslationMode.Regular);
+
+        Assert.Equal(Word.Length, hyphens.Length);
+        Assert.Matches("^[012]+$", hyphens);
+
+        // "bog-sta-ver": the table has to find at least one break, otherwise this test is not
+        // exercising hyphenation at all.
+        Assert.Contains('1', hyphens);
+    }
+
+    /// <summary>
+    /// The result must describe the word that was passed in, not a NUL terminator the wrapper
+    /// added. Unlike lou_translateString, lou_hyphenate does not clamp inlen at the first NUL:
+    /// it memcpy's exactly inlen characters, so an inflated inlen hyphenates the terminator too.
+    /// </summary>
+    [Fact]
+    public void Hyphenate_DoesNotIncludeTheNulTerminator()
+    {
+        foreach (string word in new[] { "a", "bo", "bogstaver", "hyphenation" })
+        {
+            string hyphens = LibLouis.Instance.Hyphenate(TablePaths(), word, TranslationMode.Regular);
+
+            Assert.Equal(word.Length, hyphens.Length);
+        }
+    }
+
+    /// <summary>
+    /// inlen is a widechar count. On a UCS-4 build a non-BMP character is one widechar but two
+    /// chars, so passing string.Length claims the buffer is longer than it is - and lou_hyphenate
+    /// memcpy's exactly inlen widechars out of it, with no terminator to stop at.
+    /// </summary>
+    [Theory]
+    [InlineData("bogstaver\U0001D11E")]              // one flag too many
+    [InlineData("bogstaver\U0001D11E\U0001D11E")]    // and reads past the input buffer
+    public void Hyphenate_ReturnsOneFlagPerWidecharNotPerCodeUnit(string word)
+    {
+        int expected = SafeNativeMethods.lou_charSize() == 4
+            ? word.EnumerateRunes().Count()
+            : word.Length;
+
+        string hyphens = LibLouis.Instance.Hyphenate(TablePaths(), word, TranslationMode.Regular);
+
+        Assert.Equal(expected, hyphens.Length);
+    }
+}

LibLouis.NET.Test/IndexTablesTests.cs

@@ -0,0 +1,58 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+
+using Microsoft.Extensions.Logging;
+
+using Xunit;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// lou_indexTables walks its argument until it hits a NULL pointer
+/// (<c>for (table = tables; *table; table++)</c>, metadata.c:905). A managed string[] marshals to
+/// exactly Length pointers with no terminator, so liblouis reads past the end of the array.
+/// </summary>
+public class IndexTablesTests
+{
+    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))];
+
+    /// <summary>
+    /// liblouis logs one "Analyzing table &lt;name&gt;" line per array entry it walks, so the number
+    /// of those lines is a direct measure of how far it read.
+    /// </summary>
+    /// <remarks>
+    /// Without the terminator this does not fail, it hangs: liblouis reads the managed memory
+    /// following the array as a char* and _lou_logMessage formats it with %s until it runs out of
+    /// readable memory. A regression here shows up as a test run that never finishes.
+    /// </remarks>
+    [Fact]
+    public void IndexTables_DoesNotReadPastTheEndOfTheArray()
+    {
+        string[] paths = TablePaths();
+
+        CollectingLogger logger = new();
+        ILogger previous = LibLouis.Instance.Logger;
+        LibLouis.Instance.Logger = logger;
+
+        try
+        {
+            LibLouis.Instance.IndexTables(paths);
+        }
+        finally
+        {
+            LibLouis.Instance.Logger = previous;
+        }
+
+        List<string> analyzed = [.. logger.Messages
+            .Where(m => m.StartsWith("Analyzing table ", StringComparison.Ordinal))
+            .Select(m => m["Analyzing table ".Length..])];
+
+        Assert.Equal(paths, analyzed);
+    }
+
+}

LibLouis.NET.Test/InputLengthTests.cs

@@ -0,0 +1,82 @@
+using System;
+using System.IO;
+using System.Linq;
+
+using Xunit;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// inlen is a widechar count that excludes the NUL terminator, matching the header and what
+/// upstream callers pass. These tests pin down the two properties that depend on it:
+///
+///   * The terminator is not translated as if it were text. The buffer stays NUL terminated
+///     (PrepareUCSInputBuffer's job) and lou_translateString clamps at the first NUL
+///     (<c>while (k &lt; *inlen &amp;&amp; inbufx[k]) k++;</c>, lou_translateString.c:1191), so an
+///     embedded NUL still ends the input.
+///   * Nothing is written past the position arrays the argument checks demand. liblouis
+///     overwrites *inlen with the number of characters actually consumed
+///     (lou_translateString.c:1354) before computing outputPos.
+///
+/// The wrapper previously passed input.Length + 1 here. That was safe - the clamp at :1191 and
+/// the overwrite at :1354 between them made the extra count unreachable - but it left
+/// correctness resting on two undocumented internals instead of the documented contract.
+/// </summary>
+public class InputLengthTests
+{
+    private const string Input = "Første linje. Anden linje, med kursiveret tekst. Tredje linje.";
+
+    private static readonly string[] Tables = ["da-dk-braillo.dis", "da-dk-g26.ctb"];
+
+    private static string[] TablePaths() =>
+        [.. Tables.Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))];
+
+    /// <summary>
+    /// Translate() only requires outputPosition to hold input.Length entries, so liblouis must
+    /// not write beyond that. The array is deliberately oversized and sentinel filled, so an
+    /// out-of-bounds write would be observable here instead of corrupting the heap.
+    /// </summary>
+    [Fact]
+    public void Translate_DoesNotWriteOutputPositionsPastInputLength()
+    {
+        // Not -1: liblouis pre-fills outputPos with -1 for the characters it owns, so -1 could
+        // not tell an untouched entry apart from one liblouis had written.
+        const int sentinel = int.MinValue;
+        const int slack = 8;
+
+        int outputLength = Input.Length * 4;
+
+        int[] outputPosition = new int[Input.Length + slack];
+        Array.Fill(outputPosition, sentinel);
+
+        LibLouis.Instance.Translate(
+            TablePaths(),
+            Input,
+            outputLength,
+            null,
+            null,
+            outputPosition,
+            new int[outputLength],
+            0,
+            TranslationMode.Regular);
+
+        int firstUntouched = Array.FindIndex(outputPosition, p => p == sentinel);
+
+        Assert.Equal(Input.Length, firstUntouched);
+    }
+
+    /// <summary>
+    /// The NUL terminator is not translated as if it were input text.
+    /// </summary>
+    [Fact]
+    public void Translate_DoesNotTranslateTheNulTerminator()
+    {
+        const string input = "abc";
+
+        string translated = LibLouis.Instance.Translate(
+            TablePaths(), input, input.Length * 4, null, null, TranslationMode.Regular);
+
+        Assert.DoesNotContain('\0', translated);
+        Assert.Equal(input, translated);
+    }
+}

LibLouis.NET.Test/LibLouis.NET.Test.csproj

@@ -5,6 +5,8 @@
     <Nullable>enable</Nullable>
     <IsPackable>false</IsPackable>
     <IsTestProject>true</IsTestProject>
+    <!-- The custom marshallers work in byte*, so testing them directly needs unsafe. -->
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
 
   <ItemGroup>

LibLouis.NET.Test/LogCallbackTests.cs

@@ -0,0 +1,70 @@
+using System;
+
+using Microsoft.Extensions.Logging;
+
+using Xunit;
+
+namespace LibLouis.NET.Test;
+
+/// <summary>
+/// liblouis keeps the function pointer it is handed by lou_registerLogCallback and calls it for
+/// the rest of the process's life. The managed delegate behind that pointer therefore has to stay
+/// alive for just as long: the marshalling stub only keeps it alive for the duration of the
+/// registration call itself.
+/// </summary>
+public class LogCallbackTests
+{
+    /// <summary>
+    /// Forces collections between registering the callback and provoking a native log message.
+    /// If nothing roots the delegate, the pointer liblouis holds is dangling by then.
+    /// </summary>
+    [Fact]
+    public void Logger_StillReceivesMessagesAfterGarbageCollection()
+    {
+        CollectingLogger logger = new();
+        LibLouis.Instance.Logger = logger;
+
+        for (int i = 0; i < 3; i++)
+        {
+            GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, blocking: true, compacting: true);
+            GC.WaitForPendingFinalizers();
+        }
+
+        // Any failing call makes liblouis log; a table that cannot be compiled is the simplest.
+        Assert.Throws<LibLouisException>(
+            () => LibLouis.Instance.Translate(
+                ["no-such-table-at-all.ctb"], "x", 8, null, null, TranslationMode.Regular));
+
+        Assert.NotEmpty(logger.Messages);
+    }
+
+    /// <summary>
+    /// The callback runs on a native stack. An exception thrown out of it cannot be handled by
+    /// liblouis and tears the process down, so an unmapped level must not throw.
+    /// </summary>
+    [Fact]
+    public void LogCallback_SurvivesALevelItDoesNotKnow()
+    {
+        CollectingLogger logger = new();
+        LibLouis.Instance.Logger = logger;
+
+        // 12345 is not one of the logLevels values liblouis defines.
+        NativeMethods.LoggingCallback callback = GetRegisteredCallback();
+
+        callback((LogLevel)12345, "message at an unknown level");
+    }
+
+    /// <summary>
+    /// Reaches the delegate the wrapper registered, so the test calls exactly what liblouis calls.
+    /// </summary>
+    private static NativeMethods.LoggingCallback GetRegisteredCallback()
+    {
+        object? field = typeof(LibLouis)
+            .GetField("_logCallback", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)
+            ?.GetValue(LibLouis.Instance);
+
+        Assert.NotNull(field);
+
+        return (NativeMethods.LoggingCallback)field;
+    }
+}