From 169cf1c1e1916385e61e8784af7d670ccb4862ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 00:54:08 +0200 Subject: [PATCH 01/14] fix: Stop liblouis writing past the caller's typeform array liblouis treats typeform as in/out: it reads one entry per input character, but on a successful translation it writes one entry per *output* cell (lou_translateString.c:1329). The public API sizes typeform to the input, so any translation that grows the text - which the da-dk marker tables do routinely - had native code writing past the end of a pinned managed array, corrupting the GC heap. The existing SingleMode test triggered this: 15 input characters, 20 output cells, 10 bytes written past the array. It passed only because the overwritten memory happened not to matter. Hand liblouis a scratch buffer sized for both directions and treat the caller's array as input only. Tests are run serially from now on: liblouis has process-global state and is not thread safe, but xunit parallelises across test classes, so a second test class made unsynchronised native calls race. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/AssemblyInfo.cs | 7 ++ LibLouis.NET.Test/TypeFormBufferTests.cs | 141 +++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 35 +++++- 3 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 LibLouis.NET.Test/AssemblyInfo.cs create mode 100644 LibLouis.NET.Test/TypeFormBufferTests.cs diff --git a/LibLouis.NET.Test/AssemblyInfo.cs b/LibLouis.NET.Test/AssemblyInfo.cs new file mode 100644 index 0000000..dffc93b --- /dev/null +++ b/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)] diff --git a/LibLouis.NET.Test/TypeFormBufferTests.cs b/LibLouis.NET.Test/TypeFormBufferTests.cs new file mode 100644 index 0000000..4f77f2e --- /dev/null +++ b/LibLouis.NET.Test/TypeFormBufferTests.cs @@ -0,0 +1,141 @@ +using System; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using System.Text; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// liblouis writes the typeform array back for every *output* cell, not for every input +/// character. Passing a typeform array sized to the input therefore lets native code write +/// past the end of a managed array whenever the translation grows the text - which the +/// marker tables in this repository do routinely. +/// +public class TypeFormBufferTests +{ + private const string Input = "This is a test."; + + /// Translation of with the marker tables, 20 cells for 15 characters. + private const string ExpectedOutput = "`,@this is a test.`,"; + + private static readonly string[] Tables = ["da-dk-braillo.dis", "da-dk-g16-markers.ctb"]; + + private static string[] TablePaths() => + [.. Tables.Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))]; + + /// + /// Documents the native contract that makes the overrun possible, independent of the wrapper: + /// lou_translateString writes one typeform entry per output cell. Uses a deliberately + /// oversized buffer so nothing is corrupted while we measure how far native code writes. + /// + [Fact] + public void Native_WritesOneTypeformEntryPerOutputCell() + { + int charSize = NativeShim.lou_charSize(); + Encoding encoder = charSize == 4 ? Encoding.UTF32 : Encoding.Unicode; + + int inputLength = Input.Length; + int outputLength = ExpectedOutput.Length; + + byte[] inputBuffer = encoder.GetBytes(Input + "\0"); + byte[] outputBuffer = new byte[(outputLength + 1) * charSize]; + + // Far larger than either length, so native writes stay in bounds and are observable. + // typeform is in/out: the first inputLength entries are real input (foreign language, + // matching the other tests), the rest are plain text. Neither value collides with the + // ASCII '0' / '8' that liblouis writes back, so any such entry marks a native write. + ushort[] typeform = new ushort[outputLength * 4]; + Array.Fill(typeform, (ushort)TypeForm.ForeignLanguage, 0, inputLength); + + int inLen = inputLength; + int outLen = outputLength; + + int result = NativeShim.lou_translateString( + string.Join(',', TablePaths()), inputBuffer, ref inLen, outputBuffer, ref outLen, typeform, null, 0); + + Assert.NotEqual(0, result); + Assert.Equal(ExpectedOutput, encoder.GetString(outputBuffer, 0, outLen * charSize)); + + // liblouis writes the ASCII characters '0' / '8' per output cell. + int lastWritten = Array.FindLastIndex(typeform, t => t == '0' || t == '8'); + + Assert.Equal(outLen - 1, lastWritten); + + // The point of the test: native wrote beyond the input length, so an input-sized + // managed array would have been overrun by exactly this many entries. + Assert.True( + lastWritten >= inputLength, + $"Expected native writes past input length {inputLength}, but last write was at {lastWritten}."); + } + + /// + /// The wrapper must not let native code write into - let alone past - the caller's typeform + /// array. The public contract sizes typeform to the input, so the wrapper owes the caller a + /// buffer big enough for the output. + /// + [Fact] + public void Translate_DoesNotWriteIntoCallersTypeformArray() + { + TypeForm[] typeform = new TypeForm[Input.Length]; + Array.Fill(typeform, TypeForm.ForeignLanguage); + + TypeForm[] untouched = (TypeForm[])typeform.Clone(); + + string output = LibLouis.Instance.Translate( + TablePaths(), Input, Input.Length * 2, typeform, null, TranslationMode.Regular); + + Assert.Equal(ExpectedOutput, output); + Assert.Equal(untouched, typeform); + } + + /// + /// The same overrun through the position-reporting overload. + /// + [Fact] + public void TranslateWithPositions_DoesNotWriteIntoCallersTypeformArray() + { + int outputLength = Input.Length * 2; + + TypeForm[] typeform = new TypeForm[Input.Length]; + Array.Fill(typeform, TypeForm.ForeignLanguage); + + TypeForm[] untouched = (TypeForm[])typeform.Clone(); + + TranslatedString translated = LibLouis.Instance.Translate( + TablePaths(), + Input, + outputLength, + typeform, + null, + new int[Input.Length], + new int[outputLength], + 0, + TranslationMode.Regular); + + Assert.Equal(ExpectedOutput, translated.Output); + Assert.Equal(untouched, typeform); + } + + /// + /// Raw P/Invoke used to characterise native behaviour without going through the wrapper. + /// + private static class NativeShim + { + [DllImport("liblouis", EntryPoint = "lou_charSize")] + internal static extern int lou_charSize(); + + [DllImport("liblouis", EntryPoint = "lou_translateString", CharSet = CharSet.Ansi)] + internal static extern int lou_translateString( + [MarshalAs(UnmanagedType.LPUTF8Str)] string tableList, + byte[] inbuf, + ref int inlen, + byte[] outbuf, + ref int outlen, + ushort[]? typeform, + [MarshalAs(UnmanagedType.LPUTF8Str)] string? spacing, + int mode); + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 22bb72b..197f164 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -290,13 +290,14 @@ public TranslatedString Translate( byte[] inputBuffer = PrepareUCSInputBuffer(input); byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength); + TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength); string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, formtype, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; + success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } if (!success) @@ -344,13 +345,14 @@ public string Translate(IEnumerable tableList, string input, int outputL byte[] inputBuffer = PrepareUCSInputBuffer(input); byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength); + TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength); string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_translateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, formtype, spacing, mode) > 0; + success = NativeMethods.lou_translateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } if (!success) @@ -415,13 +417,14 @@ public TranslatedString BackTranslate( byte[] inputBuffer = PrepareUCSInputBuffer(input); byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength); + TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength); string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, formtype, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; + success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } if (!success) @@ -467,13 +470,14 @@ public string BackTranslate(IEnumerable tableList, string input, int out byte[] inputBuffer = PrepareUCSInputBuffer(input); byte[] outputBuffer = PrepareUCSOutputBuffer(outputBufferLength); + TypeForm[]? typeFormBuffer = PrepareTypeFormBuffer(formtype, inputLength, outputBufferLength); string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_backTranslateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, formtype, spacing, mode) > 0; + success = NativeMethods.lou_backTranslateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } if (!success) @@ -520,6 +524,29 @@ public string Hyphenate(IEnumerable tableList, string input, Translation return hyphens; } + /// + /// Copy the caller's typeform values into a buffer that is safe to hand to liblouis. + /// + /// + /// The typeform parameter is in/out: liblouis reads one entry per input character, but on a + /// successful translation it writes one entry per *output* cell. A translation that grows the + /// text - which the marker tables do routinely - would therefore write past the end of an + /// array sized to the input, corrupting the managed heap. We give liblouis a buffer big enough + /// for both directions and treat the caller's array as input only. + /// + private static TypeForm[]? PrepareTypeFormBuffer(TypeForm[]? formtype, int inputLength, int outputLength) + { + if (formtype is null) + { + return null; + } + + TypeForm[] buffer = new TypeForm[Math.Max(inputLength, outputLength) + 1]; + formtype.AsSpan(0, Math.Min(formtype.Length, buffer.Length)).CopyTo(buffer); + + return buffer; + } + /// /// Return UCS-2/4 null terminated encoding of input. /// From 063dd9fd4d3f79e978911cbc5b70031ae7310fcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 00:57:32 +0200 Subject: [PATCH 02/14] test: Pin down why the inlen "+ 1" is safe An audit flagged inputLength = input.Length + 1 as counting the NUL terminator as translatable input, and as widening the outputPos write past the array size the argument checks demand. Reading liblouis shows neither happens: * lou_translateString clamps the length at the first NUL (lou_translateString.c:1191), so the terminator is never translated. * It then overwrites *inlen with the number of characters actually consumed (lou_translateString.c:1354) before computing outputPos, so the inflated value never reaches the position loops. Both properties rely on the buffer really being NUL terminated, and neither is visible at the call site, so add tests and a comment rather than a change. The tests use int.MinValue as the sentinel: liblouis pre-fills outputPos with -1, which would mask an out-of-bounds write. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/InputLengthTests.cs | 81 +++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 20 +++++++ 2 files changed, 101 insertions(+) create mode 100644 LibLouis.NET.Test/InputLengthTests.cs diff --git a/LibLouis.NET.Test/InputLengthTests.cs b/LibLouis.NET.Test/InputLengthTests.cs new file mode 100644 index 0000000..875cd06 --- /dev/null +++ b/LibLouis.NET.Test/InputLengthTests.cs @@ -0,0 +1,81 @@ +using System; +using System.IO; +using System.Linq; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// The wrapper passes inlen as input.Length + 1, which looks like it counts the NUL terminator +/// as a character to translate. It does not, and these tests pin that down so the "+ 1" is not +/// removed - or relied on - by mistake: +/// +/// * lou_translateString clamps the length at the first NUL +/// (while (k < *inlen && inbufx[k]) k++;, lou_translateString.c:1191), so the +/// terminator is never translated. +/// * It then overwrites *inlen with the number of characters actually consumed +/// (lou_translateString.c:1354) before computing outputPos, so the inflated value cannot +/// reach the position loops and cannot push a write past the caller's array. +/// +/// Both properties depend on the input buffer really being NUL terminated, which is +/// PrepareUCSInputBuffer's job. +/// +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))]; + + /// + /// 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. + /// + [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); + } + + /// + /// The NUL terminator is not translated as if it were input text. + /// + [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); + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 197f164..73184ad 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -285,6 +285,11 @@ public TranslatedString Translate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } + // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis + // clamps the length at the first NUL and then overwrites inlen with the number of + // characters it actually consumed, before it computes any position mapping. The + // terminator is therefore never translated and never widens a position array write. + // See InputLengthTests. int inputLength = input.Length + 1; int outputBufferLength = outputLength; @@ -340,6 +345,11 @@ public string Translate(IEnumerable tableList, string input, int outputL throw new ArgumentException("Spacing must be the same length as input or null"); } + // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis + // clamps the length at the first NUL and then overwrites inlen with the number of + // characters it actually consumed, before it computes any position mapping. The + // terminator is therefore never translated and never widens a position array write. + // See InputLengthTests. int inputLength = input.Length + 1; int outputBufferLength = outputLength; @@ -412,6 +422,11 @@ public TranslatedString BackTranslate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } + // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis + // clamps the length at the first NUL and then overwrites inlen with the number of + // characters it actually consumed, before it computes any position mapping. The + // terminator is therefore never translated and never widens a position array write. + // See InputLengthTests. int inputLength = input.Length + 1; int outputBufferLength = outputLength; @@ -465,6 +480,11 @@ public string BackTranslate(IEnumerable tableList, string input, int out throw new ArgumentException("Spacing must be the same length as input or null"); } + // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis + // clamps the length at the first NUL and then overwrites inlen with the number of + // characters it actually consumed, before it computes any position mapping. The + // terminator is therefore never translated and never widens a position array write. + // See InputLengthTests. int inputLength = input.Length + 1; int outputBufferLength = outputLength; From b4fa2907a39900398e838d1e60583374b0a73007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 01:13:42 +0200 Subject: [PATCH 03/14] fix: NULL-terminate the array handed to lou_indexTables lou_indexTables walks its argument until it reads a null pointer (metadata.c:905), but a managed string[] marshals to exactly Length pointers with no terminator. liblouis therefore read whatever managed memory followed the array and passed it to _lou_logMessage as a char*. This is not theoretical: calling IndexTables hangs the process. A stack sample of the wedged test host shows it parked in lou_indexTables -> _lou_logMessage formatting %s against a garbage pointer until it runs out of readable memory. Append the terminator and let the signature say so (string?[]). The regression test asserts liblouis analyzed exactly the tables it was given. Note that a regression does not fail it, it hangs it. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/IndexTablesTests.cs | 76 +++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 9 +++- LibLouis.NET/NativeMethod.cs | 9 +++- 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 LibLouis.NET.Test/IndexTablesTests.cs diff --git a/LibLouis.NET.Test/IndexTablesTests.cs b/LibLouis.NET.Test/IndexTablesTests.cs new file mode 100644 index 0000000..c7265b7 --- /dev/null +++ b/LibLouis.NET.Test/IndexTablesTests.cs @@ -0,0 +1,76 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; + +using Microsoft.Extensions.Logging; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// lou_indexTables walks its argument until it hits a NULL pointer +/// (for (table = tables; *table; table++), metadata.c:905). A managed string[] marshals to +/// exactly Length pointers with no terminator, so liblouis reads past the end of the array. +/// +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))]; + + /// + /// liblouis logs one "Analyzing table <name>" line per array entry it walks, so the number + /// of those lines is a direct measure of how far it read. + /// + /// + /// 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. + /// + [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 analyzed = [.. logger.Messages + .Where(m => m.StartsWith("Analyzing table ", StringComparison.Ordinal)) + .Select(m => m["Analyzing table ".Length..])]; + + Assert.Equal(paths, analyzed); + } + + private sealed class CollectingLogger : ILogger + { + public List Messages { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true; + + public void Log( + Microsoft.Extensions.Logging.LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Messages.Add(formatter(state, exception)); + } + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 73184ad..7a7d295 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -173,9 +173,16 @@ public string? DataPath /// tables must be an IEnumerable of file names. public void IndexTables(IEnumerable tables) { + ArgumentNullException.ThrowIfNull(tables); + + // liblouis walks the array until it reads a null pointer, so it needs a terminator on top + // of the table names. Without it, it reads whatever managed memory follows the array and + // hands it to _lou_logMessage as a string. + string?[] nullTerminated = [.. tables, null]; + lock (_lock) { - NativeMethods.lou_indexTables(tables.ToArray()); + NativeMethods.lou_indexTables(nullTerminated); } } diff --git a/LibLouis.NET/NativeMethod.cs b/LibLouis.NET/NativeMethod.cs index 4eaf17f..623cc63 100644 --- a/LibLouis.NET/NativeMethod.cs +++ b/LibLouis.NET/NativeMethod.cs @@ -186,9 +186,16 @@ internal static partial int lou_backTranslateString( [LibraryImport("liblouis", EntryPoint = "lou_checkTable", StringMarshalling = StringMarshalling.Utf8)] internal static partial int lou_checkTable(string tableList); + /// + /// Parses, analyzes and indexes the given tables. + /// + /// + /// Must be NULL terminated: liblouis walks the array until it reads a null pointer, so the + /// final element has to be . + /// [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_indexTables", StringMarshalling = StringMarshalling.Utf8)] - internal static partial void lou_indexTables(string[] tables); + internal static partial void lou_indexTables(string?[] tables); [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_findTable", StringMarshalling = StringMarshalling.Utf8)] From 36ba9c0b047cc895b70f2a3c262d2bdc3b42485f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 01:24:37 +0200 Subject: [PATCH 04/14] fix: Marshal the lou_hyphenate output buffer as a byte array hyphens was declared "ref string". With source-generated interop that passes a byte**, a pointer to the stub's own pointer slot. liblouis takes it as the char* output buffer and writes inlen + 1 bytes of '0'/'1' flags through it, over the stub's stack, and the stub then marshals a result string back from the clobbered pointer. Hyphenate could never have worked; it was simply untested. Calling it wedges the process: the test host is left unkillable in uninterruptible exit, which is what memory corruption looks like from the outside. Also: * inlen must not count the NUL terminator here. lou_hyphenate memcpy's exactly inlen characters instead of stopping at a NUL the way the translate functions do, so the old input.Length + 1 hyphenated the terminator as if it were a letter. * Reject input of 100 characters or more up front. liblouis hyphenates through a fixed HYPHSTRING buffer and refuses longer input, which surfaced as an unexplained hyphenation failure. * ArgumentNullException.ThrowIfNullOrEmpty(nameof(input)) validated the literal "input", so it could never fire. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/HyphenateTests.cs | 51 +++++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 36 ++++++++++++++++---- LibLouis.NET/NativeMethod.cs | 15 +++++++-- 3 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 LibLouis.NET.Test/HyphenateTests.cs diff --git a/LibLouis.NET.Test/HyphenateTests.cs b/LibLouis.NET.Test/HyphenateTests.cs new file mode 100644 index 0000000..60c1946 --- /dev/null +++ b/LibLouis.NET.Test/HyphenateTests.cs @@ -0,0 +1,51 @@ +using System; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// lou_hyphenate takes a caller-allocated char *hyphens buffer and writes inlen + 1 bytes +/// into it: '0' or '1' per character, plus a terminator (lou_translateString.c:4080). +/// +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); + } + + /// + /// 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. + /// + [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); + } + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 7a7d295..774fdda 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -522,25 +522,40 @@ public string BackTranslate(IEnumerable tableList, string input, int out /// If it does not, the function does nothing. /// /// - /// + /// The word to hyphenate. Must be shorter than 100 characters. /// - /// + /// + /// One character per character of : '1' where the word may be broken, + /// '0' where it may not, '2' after an existing hyphen. + /// /// public string Hyphenate(IEnumerable tableList, string input, TranslationMode mode) { ArgumentNullException.ThrowIfNull(tableList); - ArgumentNullException.ThrowIfNullOrEmpty(nameof(input)); + ArgumentException.ThrowIfNullOrEmpty(input); + + // liblouis rejects anything from HYPHSTRING characters up, and would otherwise report it + // as an ordinary hyphenation failure. + if (input.Length >= MaxHyphenationLength) + { + throw new ArgumentException( + $"{nameof(input)} must be shorter than {MaxHyphenationLength} characters.", nameof(input)); + } string tables = string.Join(',', tableList); - string hyphens = new('\0', input.Length + 1); + + // liblouis writes one flag per character plus a NUL terminator into a caller-allocated + // char buffer. inlen must not count the terminator: lou_hyphenate memcpy's exactly inlen + // characters rather than stopping at a NUL the way the translate functions do. + byte[] hyphens = new byte[input.Length + 1]; byte[] inputBuffer = PrepareUCSInputBuffer(input); bool success; - + lock (_lock) { - success = NativeMethods.lou_hyphenate(tables, inputBuffer, input.Length + 1, ref hyphens, mode) > 0; + success = NativeMethods.lou_hyphenate(tables, inputBuffer, input.Length, hyphens, mode) > 0; } if (!success) @@ -548,9 +563,16 @@ public string Hyphenate(IEnumerable tableList, string input, Translation throw new LibLouisException($"Hyphenation failed {_lastLogMessage}"); } - return hyphens; + // The flags are ASCII digits; the trailing terminator is not part of the result. + return Encoding.ASCII.GetString(hyphens, 0, input.Length); } + /// + /// liblouis hyphenates into a fixed 100 character buffer (HYPHSTRING) and refuses any input + /// that would not fit. + /// + private const int MaxHyphenationLength = 100; + /// /// Copy the caller's typeform values into a buffer that is safe to hand to liblouis. /// diff --git a/LibLouis.NET/NativeMethod.cs b/LibLouis.NET/NativeMethod.cs index 623cc63..da21f0f 100644 --- a/LibLouis.NET/NativeMethod.cs +++ b/LibLouis.NET/NativeMethod.cs @@ -130,13 +130,22 @@ internal static partial int lou_backTranslateString( /// /// Contains a hyphenation table. /// length of the character string in inbuf. - /// inlen is the length of the character string in inbuf - /// array of characters and must be of size inlen + 1 (to account for the NULL terminator). + /// + /// The number of characters in inbuf. Unlike the translate functions, lou_hyphenate does not + /// stop at a NUL: it copies exactly inlen characters, so this must not count the terminator. + /// It must also be less than 100 (HYPHSTRING), or liblouis refuses the call. + /// + /// + /// Caller-allocated output buffer of at least inlen + 1 bytes. liblouis writes one ASCII + /// '0' / '1' / '2' per character plus a NUL terminator. It is a plain char buffer, so it must + /// be marshalled as a byte array - a string would pass a pointer to a pointer and liblouis + /// would write over the marshalling stub's own stack. + /// /// /// 0 if error, 1 if success. [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_hyphenate", StringMarshalling = StringMarshalling.Utf8)] - internal static partial int lou_hyphenate(string tableList, byte[] inbuf, int inlen, ref string hyphens, TranslationMode mode); + internal static partial int lou_hyphenate(string tableList, byte[] inbuf, int inlen, byte[] hyphens, TranslationMode mode); /// /// This function enables you to compile a table entry on the fly at run-time. From 0773d7786044e9a96e413d83371d48c57dc74e3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 01:45:15 +0200 Subject: [PATCH 05/14] fix: Stop freeing the strings liblouis hands back lou_version, lou_getDataPath and lou_setDataPath return a pointer into static storage inside liblouis. With StringMarshalling.Utf8 on the return value the generated stub frees whatever came back, so setting DataPath aborted the process outright: ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED malloc_report -> malloc_vreport -> abort UTF8StringNoFreeMarshaller already existed for this, but was only wired up to lou_version, and it could not be applied more widely as written: * ConvertToUnmanaged reassigned its Span local to a fresh managed array instead of encoding into the buffer it had just allocated, so it returned uninitialised native memory and leaked the allocation. It also threw on empty strings. Nothing exercised it, because only the return path was ever used - but lou_setDataPath takes a string parameter, so applying the marshaller would have started feeding liblouis garbage paths. * MarshalMode.Default offered it for parameters too, where never freeing is a leak rather than a fix. So restrict it to ManagedToUnmanagedOut, drop ConvertToUnmanaged entirely, and let parameters keep the built-in Utf8StringMarshaller. ConvertToManaged becomes Marshal.PtrToStringUTF8, which also drops the int.MaxValue span that threw when no terminator was found. lou_findTable is documented as caller-frees, but our Windows binaries are mingw-w64 and allocate from msvcrt.dll while .NET frees through ucrtbase.dll. Freeing across those heaps corrupts them, so it uses the same marshaller and leaks a bounded number of small strings instead. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/LibLouis.NET.Test.csproj | 2 + .../ReturnedStringOwnershipTests.cs | 41 ++++++++++++ .../UTF8StringNoFreeMarshallerTests.cs | 66 +++++++++++++++++++ LibLouis.NET/NativeMethod.cs | 28 ++++++-- LibLouis.NET/UTF8StringNoFreeMarshaller.cs | 64 ++++++++---------- 5 files changed, 161 insertions(+), 40 deletions(-) create mode 100644 LibLouis.NET.Test/ReturnedStringOwnershipTests.cs create mode 100644 LibLouis.NET.Test/UTF8StringNoFreeMarshallerTests.cs diff --git a/LibLouis.NET.Test/LibLouis.NET.Test.csproj b/LibLouis.NET.Test/LibLouis.NET.Test.csproj index 8dee374..7138be2 100644 --- a/LibLouis.NET.Test/LibLouis.NET.Test.csproj +++ b/LibLouis.NET.Test/LibLouis.NET.Test.csproj @@ -5,6 +5,8 @@ enable false true + + true diff --git a/LibLouis.NET.Test/ReturnedStringOwnershipTests.cs b/LibLouis.NET.Test/ReturnedStringOwnershipTests.cs new file mode 100644 index 0000000..4c3a811 --- /dev/null +++ b/LibLouis.NET.Test/ReturnedStringOwnershipTests.cs @@ -0,0 +1,41 @@ +using System; +using System.IO; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// liblouis owns every string it returns, so the wrapper must not hand those pointers to the +/// marshaller's Free. +/// +/// * 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. +/// +public class ReturnedStringOwnershipTests +{ + /// + /// Setting the data path returns the static buffer, which the marshaller would then free. + /// + /// + /// The path is the test output directory rather than something arbitrary, because the data + /// path takes part in resolving relative table names and other tests rely on that. + /// + [Fact] + public void DataPath_RoundTripsWithoutFreeingLiblouisMemory() + { + string path = Path.TrimEndingDirectorySeparator(AppContext.BaseDirectory); + + LibLouis.Instance.DataPath = path; + + Assert.Equal(path, LibLouis.Instance.DataPath); + + // Reading it again returns the same static buffer; a stale free shows up here as a crash + // or as garbage. + Assert.Equal(path, LibLouis.Instance.DataPath); + } +} diff --git a/LibLouis.NET.Test/UTF8StringNoFreeMarshallerTests.cs b/LibLouis.NET.Test/UTF8StringNoFreeMarshallerTests.cs new file mode 100644 index 0000000..0373381 --- /dev/null +++ b/LibLouis.NET.Test/UTF8StringNoFreeMarshallerTests.cs @@ -0,0 +1,66 @@ +using System; +using System.Runtime.InteropServices; +using System.Text; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// The marshaller used for strings liblouis owns. It only converts inbound: not freeing is +/// correct for memory liblouis allocated, and would be a leak for buffers we allocate ourselves, +/// so it is restricted to return values. +/// +public unsafe class UTF8StringNoFreeMarshallerTests +{ + [Theory] + [InlineData("")] + [InlineData("a")] + [InlineData("tables/da-dk-g26.ctb")] + [InlineData("Første linje")] // multi-byte UTF-8 + [InlineData("\U0001D11E")] // non-BMP, surrogate pair on the managed side + public void ConvertToManaged_ReadsNulTerminatedUtf8(string value) + { + byte[] utf8 = Encoding.UTF8.GetBytes(value + "\0"); + + fixed (byte* unmanaged = utf8) + { + Assert.Equal(value, UTF8StringNoFreeMarshaller.ConvertToManaged(unmanaged)); + } + } + + /// + /// The string must stop at the terminator, not run on into whatever follows it. + /// + [Fact] + public void ConvertToManaged_StopsAtTheTerminator() + { + byte[] utf8 = Encoding.UTF8.GetBytes("abc\0trailing garbage"); + + fixed (byte* unmanaged = utf8) + { + Assert.Equal("abc", UTF8StringNoFreeMarshaller.ConvertToManaged(unmanaged)); + } + } + + [Fact] + public void ConvertToManaged_MapsNullPointerToNull() + { + Assert.Null(UTF8StringNoFreeMarshaller.ConvertToManaged(null)); + } + + /// + /// Free must leave the memory alone. If it released it, the allocator would abort on the + /// second release here. + /// + [Fact] + public void Free_DoesNotReleaseTheMemory() + { + byte* buffer = (byte*)NativeMemory.Alloc(4); + + UTF8StringNoFreeMarshaller.Free(buffer); + + // Ours to release, and still ours after Free. + NativeMemory.Free(buffer); + } +} diff --git a/LibLouis.NET/NativeMethod.cs b/LibLouis.NET/NativeMethod.cs index da21f0f..95f55df 100644 --- a/LibLouis.NET/NativeMethod.cs +++ b/LibLouis.NET/NativeMethod.cs @@ -1,4 +1,5 @@ using System.Runtime.InteropServices; +using System.Runtime.InteropServices.Marshalling; namespace LibLouis.NET; @@ -12,7 +13,8 @@ public static partial class NativeMethods /// /// LibLouis version. [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] - [LibraryImport("liblouis", EntryPoint = "lou_version", StringMarshalling = StringMarshalling.Custom, StringMarshallingCustomType = typeof(UTF8StringNoFreeMarshaller))] + [LibraryImport("liblouis", EntryPoint = "lou_version")] + [return: MarshalUsing(typeof(UTF8StringNoFreeMarshaller))] internal static partial string lou_version(); /// @@ -183,13 +185,23 @@ internal static partial int lou_backTranslateString( [LibraryImport("liblouis", EntryPoint = "lou_registerLogCallback")] internal static partial void lou_registerLogCallback(LoggingCallback callback); + /// + /// A pointer into static storage inside liblouis, or if the path was + /// never set. Must not be freed. + /// [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] - [LibraryImport("liblouis", EntryPoint = "lou_getDataPath", StringMarshalling = StringMarshalling.Utf8)] - internal static partial string lou_getDataPath(); + [LibraryImport("liblouis", EntryPoint = "lou_getDataPath")] + [return: MarshalUsing(typeof(UTF8StringNoFreeMarshaller))] + internal static partial string? lou_getDataPath(); + /// + /// A pointer into static storage inside liblouis, or if the path was + /// rejected. Must not be freed. + /// [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_setDataPath", StringMarshalling = StringMarshalling.Utf8)] - internal static partial string lou_setDataPath(string path); + [return: MarshalUsing(typeof(UTF8StringNoFreeMarshaller))] + internal static partial string? lou_setDataPath(string path); [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_checkTable", StringMarshalling = StringMarshalling.Utf8)] @@ -206,9 +218,15 @@ internal static partial int lou_backTranslateString( [LibraryImport("liblouis", EntryPoint = "lou_indexTables", StringMarshalling = StringMarshalling.Utf8)] internal static partial void lou_indexTables(string?[] tables); + /// + /// The best matching table name, or when there is no match. liblouis + /// documents this as the caller's to free, but the memory comes from liblouis's own C runtime + /// - see for why we leak it instead. + /// [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_findTable", StringMarshalling = StringMarshalling.Utf8)] - internal static partial string lou_findTable(string query); + [return: MarshalUsing(typeof(UTF8StringNoFreeMarshaller))] + internal static partial string? lou_findTable(string query); [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] [LibraryImport("liblouis", EntryPoint = "lou_compileString", StringMarshalling = StringMarshalling.Utf8)] diff --git a/LibLouis.NET/UTF8StringNoFreeMarshaller.cs b/LibLouis.NET/UTF8StringNoFreeMarshaller.cs index 17fa844..0b5e470 100644 --- a/LibLouis.NET/UTF8StringNoFreeMarshaller.cs +++ b/LibLouis.NET/UTF8StringNoFreeMarshaller.cs @@ -1,49 +1,43 @@ -using System; using System.Runtime.InteropServices; using System.Runtime.InteropServices.Marshalling; -using System.Text; namespace LibLouis.NET; -[CustomMarshaller(typeof(string), MarshalMode.Default, typeof(UTF8StringNoFreeMarshaller))] -public unsafe static class UTF8StringNoFreeMarshaller +/// +/// Marshals a UTF-8 string that liblouis owns, without freeing it. +/// +/// +/// Several liblouis functions return a char * the caller must not release: lou_version, +/// lou_getDataPath and lou_setDataPath all hand back a pointer into static storage inside the +/// library. The default UTF-8 marshalling frees whatever the callee returned, which for those +/// pointers aborts the process ("pointer being freed was not allocated"). +/// +/// It is deliberately restricted to - return +/// values and out parameters. Not freeing is only correct for memory we did not allocate; +/// applying it to an input parameter would leak the buffer allocated for every call, so +/// parameters keep using the built-in . +/// +/// liblouis also has functions whose result the caller *is* expected to free (lou_findTable, +/// lou_findTables, lou_getTableInfo, lou_listTables). Those use this marshaller too: the Windows +/// binaries are built with mingw-w64 and allocate from msvcrt.dll while .NET frees through +/// ucrtbase.dll, so releasing that memory from managed code would corrupt the heap. Leaking a +/// bounded number of small strings is the safer trade. +/// +[CustomMarshaller(typeof(string), MarshalMode.ManagedToUnmanagedOut, typeof(UTF8StringNoFreeMarshaller))] +public static unsafe class UTF8StringNoFreeMarshaller { - public const byte NullTerminator = (byte)0; - - public static byte* ConvertToUnmanaged(string? managedString) - { - if (managedString is null) - { - return null; - } - - int unmanagedLength = Encoding.UTF8.GetByteCount(managedString) + 1; - byte* bufferPointer = (byte*)NativeMemory.Alloc((nuint)unmanagedLength); - Span byteSpan = new(bufferPointer, unmanagedLength); - - byteSpan = Encoding.UTF8.GetBytes(managedString); - byteSpan[^1] = NullTerminator; - - return bufferPointer; - } - - + /// + /// Copies the NUL terminated UTF-8 string at into a managed string. + /// public static string? ConvertToManaged(byte* unmanaged) { - if (unmanaged == null) - { - return null; - } - - Span stringSpan = new(unmanaged, int.MaxValue); - int length = stringSpan.IndexOf(NullTerminator); - - return Encoding.UTF8.GetString(unmanaged, length); + return Marshal.PtrToStringUTF8((nint)unmanaged); } - + /// + /// Deliberately does nothing: the string belongs to liblouis. + /// public static void Free(byte* unmanaged) { - // Do nothing, not caller's responsiblity to free it. } } From e05506b84cd0148b72ba1dd69c46c99aab440e47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 01:56:52 +0200 Subject: [PATCH 06/14] fix: Keep the liblouis log callback delegate alive lou_registerLogCallback was handed a delegate created from a method group that nothing kept a reference to. The interop stub only keeps it alive for the duration of the registration call, but liblouis holds the function pointer for the rest of the process's life, so the first native log message after a collection killed the process: Process terminated. A callback was made on a garbage collected delegate of type 'LibLouis.NET.NativeMethods+LoggingCallback::Invoke' Root it in a field, in LibLouis and in the static Logging helper, which had the same problem for callbacks supplied by callers. LogCallback also indexed the level map directly, so a level liblouis does not currently define would have thrown KeyNotFoundException out of a native callback - undefined behaviour rather than an error. Map unknown levels to Information and let nothing escape. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/CollectingLogger.cs | 30 ++++++++++++ LibLouis.NET.Test/IndexTablesTests.cs | 18 ------- LibLouis.NET.Test/LogCallbackTests.cs | 70 +++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 43 +++++++++++++--- LibLouis.NET/Logging.cs | 15 +++++- 5 files changed, 150 insertions(+), 26 deletions(-) create mode 100644 LibLouis.NET.Test/CollectingLogger.cs create mode 100644 LibLouis.NET.Test/LogCallbackTests.cs diff --git a/LibLouis.NET.Test/CollectingLogger.cs b/LibLouis.NET.Test/CollectingLogger.cs new file mode 100644 index 0000000..9049579 --- /dev/null +++ b/LibLouis.NET.Test/CollectingLogger.cs @@ -0,0 +1,30 @@ +using System; +using System.Collections.Generic; + +using Microsoft.Extensions.Logging; + +namespace LibLouis.NET.Test; + +/// +/// Captures everything liblouis logs, so tests can assert on what the native side reported. +/// +internal sealed class CollectingLogger : ILogger +{ + public List Messages { get; } = []; + + public IDisposable? BeginScope(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( + Microsoft.Extensions.Logging.LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Messages.Add(formatter(state, exception)); + } +} diff --git a/LibLouis.NET.Test/IndexTablesTests.cs b/LibLouis.NET.Test/IndexTablesTests.cs index c7265b7..8f0f8fa 100644 --- a/LibLouis.NET.Test/IndexTablesTests.cs +++ b/LibLouis.NET.Test/IndexTablesTests.cs @@ -55,22 +55,4 @@ public void IndexTables_DoesNotReadPastTheEndOfTheArray() Assert.Equal(paths, analyzed); } - private sealed class CollectingLogger : ILogger - { - public List Messages { get; } = []; - - public IDisposable? BeginScope(TState state) where TState : notnull => null; - - public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true; - - public void Log( - Microsoft.Extensions.Logging.LogLevel logLevel, - EventId eventId, - TState state, - Exception? exception, - Func formatter) - { - Messages.Add(formatter(state, exception)); - } - } } diff --git a/LibLouis.NET.Test/LogCallbackTests.cs b/LibLouis.NET.Test/LogCallbackTests.cs new file mode 100644 index 0000000..507c7bc --- /dev/null +++ b/LibLouis.NET.Test/LogCallbackTests.cs @@ -0,0 +1,70 @@ +using System; + +using Microsoft.Extensions.Logging; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// 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. +/// +public class LogCallbackTests +{ + /// + /// 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. + /// + [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( + () => LibLouis.Instance.Translate( + ["no-such-table-at-all.ctb"], "x", 8, null, null, TranslationMode.Regular)); + + Assert.NotEmpty(logger.Messages); + } + + /// + /// 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. + /// + [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"); + } + + /// + /// Reaches the delegate the wrapper registered, so the test calls exactly what liblouis calls. + /// + 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; + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 774fdda..4351eec 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -48,6 +48,17 @@ public class LibLouis : IDisposable private string _lastLogMessage = string.Empty; + /// + /// Roots the delegate behind the function pointer liblouis holds. + /// + /// + /// The interop stub only keeps the delegate alive for the duration of the registration call, + /// but liblouis keeps calling the pointer for the rest of the process's life. Without a + /// reference here the delegate is collected and the next native log message kills the process + /// with "A callback was made on a garbage collected delegate". + /// + private readonly NativeMethods.LoggingCallback _logCallback; + static LibLouis() { Instance = new LibLouis(); @@ -65,7 +76,8 @@ private LibLouis() }; // Register managed log callback, so we can give reasonable exception messages. - NativeMethods.lou_registerLogCallback(LogCallback); + _logCallback = LogCallback; + NativeMethods.lou_registerLogCallback(_logCallback); } // https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/unmanaged @@ -95,18 +107,37 @@ private void SetLogger(ILogger logger) { _logger = logger; NativeMethods.lou_setLogLevel(LogLevel.All); - NativeMethods.lou_registerLogCallback(LogCallback); + NativeMethods.lou_registerLogCallback(_logCallback); } } + /// + /// Called by liblouis, on a native stack. + /// + /// + /// Nothing may be thrown out of here. liblouis has no way to handle a managed exception, and + /// letting one unwind through its frames tears the process down. + /// private void LogCallback(LogLevel level, string message) { - Microsoft.Extensions.Logging.LogLevel l = LogLevels[level]; - _lastLogMessage = message; + try + { + _lastLogMessage = message; + + // liblouis is free to introduce log levels we have no mapping for. + if (!LogLevels.TryGetValue(level, out Microsoft.Extensions.Logging.LogLevel l)) + { + l = Microsoft.Extensions.Logging.LogLevel.Information; + } - if (_logger.IsEnabled(l)) + if (_logger.IsEnabled(l)) + { + _logger.Log(l, message); + } + } + catch { - _logger.Log(l, message); + // A logger that throws must not become a native crash. } } diff --git a/LibLouis.NET/Logging.cs b/LibLouis.NET/Logging.cs index a4a077d..4114e00 100644 --- a/LibLouis.NET/Logging.cs +++ b/LibLouis.NET/Logging.cs @@ -1,10 +1,21 @@ -namespace LibLouis.NET; +using System; + +namespace LibLouis.NET; public static class Logging { + /// + /// Roots the delegate behind the function pointer liblouis holds. Callers routinely pass a + /// method group, which would otherwise be collected while liblouis still calls it. + /// + private static NativeMethods.LoggingCallback? _callback; + public static void SetCallback(NativeMethods.LoggingCallback value) { - NativeMethods.lou_registerLogCallback(value); + ArgumentNullException.ThrowIfNull(value); + + _callback = value; + NativeMethods.lou_registerLogCallback(_callback); } private static LogLevel _logLevel = LogLevel.Off; From fbef3c87f2006aea132123b1ef25ff4d76fc6010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 02:02:36 +0200 Subject: [PATCH 07/14] fix: Count lengths in widechars, not UTF-16 code units On a UCS-4 build a liblouis widechar holds a whole Unicode character, so a non-BMP character is one widechar but two chars of a .NET string. The input buffer was sized by encoding the string, but the length passed alongside it was string.Length, which overstates it. lou_dotsToChar and lou_charToDots read and write exactly the count they are given, with no NUL clamping (lou_translateString.c:4142), so CharactersToDots on a two character non-BMP string had liblouis walk four widechars through a three widechar buffer - eight bytes past the end - and return four cells for two characters. Our shipped binaries are UCS-4, so this was live. The translate functions were not affected, because they clamp at the terminator, but they now count in the same unit so the two cannot drift apart again. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/NativeShim.cs | 26 ++++++++ LibLouis.NET.Test/NonBmpTests.cs | 70 +++++++++++++++++++++ LibLouis.NET.Test/TypeFormBufferTests.cs | 19 ------ LibLouis.NET/LibLouis.cs | 80 ++++++++++++++---------- 4 files changed, 144 insertions(+), 51 deletions(-) create mode 100644 LibLouis.NET.Test/NativeShim.cs create mode 100644 LibLouis.NET.Test/NonBmpTests.cs diff --git a/LibLouis.NET.Test/NativeShim.cs b/LibLouis.NET.Test/NativeShim.cs new file mode 100644 index 0000000..575cede --- /dev/null +++ b/LibLouis.NET.Test/NativeShim.cs @@ -0,0 +1,26 @@ +using System.Runtime.InteropServices; + +namespace LibLouis.NET.Test; + +/// +/// Raw P/Invoke used to characterise native behaviour without going through the wrapper. +/// +internal static class NativeShim +{ + /// + /// Bytes per liblouis widechar: 2 for a UCS-2 build, 4 for UCS-4. + /// + [DllImport("liblouis", EntryPoint = "lou_charSize")] + internal static extern int lou_charSize(); + + [DllImport("liblouis", EntryPoint = "lou_translateString")] + internal static extern int lou_translateString( + [MarshalAs(UnmanagedType.LPUTF8Str)] string tableList, + byte[] inbuf, + ref int inlen, + byte[] outbuf, + ref int outlen, + ushort[]? typeform, + [MarshalAs(UnmanagedType.LPUTF8Str)] string? spacing, + int mode); +} diff --git a/LibLouis.NET.Test/NonBmpTests.cs b/LibLouis.NET.Test/NonBmpTests.cs new file mode 100644 index 0000000..241147e --- /dev/null +++ b/LibLouis.NET.Test/NonBmpTests.cs @@ -0,0 +1,70 @@ +using System; +using System.IO; +using System.Linq; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// On a UCS-4 build a liblouis widechar holds a whole Unicode character, so a non-BMP character +/// occupies one widechar but two chars of a .NET string. Passing string.Length as a widechar +/// count therefore overstates the length of the buffer. +/// +/// lou_translateString survives that, because it clamps at the NUL terminator. lou_dotsToChar and +/// lou_charToDots do not clamp: they read and write exactly the count they are given +/// (lou_translateString.c:4142), so a count in the wrong unit reads past the input buffer. +/// +public class NonBmpTests +{ + /// U+1D11E MUSICAL SYMBOL G CLEF - one character, two UTF-16 code units. + private const string NonBmp = "\U0001D11E\U0001D11E"; + + 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))]; + + /// + /// How many widechars liblouis sees for : whole characters on a UCS-4 + /// build, UTF-16 code units on a UCS-2 one. + /// + private static int ExpectedCells(string value) + { + return NativeShim.lou_charSize() == 4 + ? value.EnumerateRunes().Count() + : value.Length; + } + + [Fact] + public void CharactersToDots_ProducesOneCellPerWidecharNotPerCodeUnit() + { + string dots = LibLouis.Instance.CharactersToDots(TablePaths(), NonBmp); + + Assert.Equal(ExpectedCells(NonBmp), dots.Length); + } + + [Fact] + public void DotsToCharacters_ProducesOneCharacterPerCell() + { + string dots = LibLouis.Instance.CharactersToDots(TablePaths(), NonBmp); + + string roundTripped = LibLouis.Instance.DotsToCharacters(TablePaths(), dots); + + Assert.Equal(dots.Length, roundTripped.Length); + } + + /// + /// BMP text must keep behaving exactly as before: there string.Length and the widechar count + /// agree, so this guards the common case against the fix. + /// + [Fact] + public void CharactersToDots_IsUnchangedForBmpText() + { + const string input = "abc"; + + string dots = LibLouis.Instance.CharactersToDots(TablePaths(), input); + + Assert.Equal(input.Length, dots.Length); + } +} diff --git a/LibLouis.NET.Test/TypeFormBufferTests.cs b/LibLouis.NET.Test/TypeFormBufferTests.cs index 4f77f2e..721a0a2 100644 --- a/LibLouis.NET.Test/TypeFormBufferTests.cs +++ b/LibLouis.NET.Test/TypeFormBufferTests.cs @@ -119,23 +119,4 @@ public void TranslateWithPositions_DoesNotWriteIntoCallersTypeformArray() Assert.Equal(untouched, typeform); } - /// - /// Raw P/Invoke used to characterise native behaviour without going through the wrapper. - /// - private static class NativeShim - { - [DllImport("liblouis", EntryPoint = "lou_charSize")] - internal static extern int lou_charSize(); - - [DllImport("liblouis", EntryPoint = "lou_translateString", CharSet = CharSet.Ansi)] - internal static extern int lou_translateString( - [MarshalAs(UnmanagedType.LPUTF8Str)] string tableList, - byte[] inbuf, - ref int inlen, - byte[] outbuf, - ref int outlen, - ushort[]? typeform, - [MarshalAs(UnmanagedType.LPUTF8Str)] string? spacing, - int mode); - } } diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 4351eec..1de6f03 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -226,15 +226,17 @@ public string DotsToCharacters(IEnumerable tableList, string input) { ArgumentNullException.ThrowIfNull(input, nameof(input)); + int length = CountUCSCharacters(input); + byte[] inputBuffer = PrepareUCSInputBuffer(input); - byte[] outputBuffer = PrepareUCSOutputBuffer(input.Length); + byte[] outputBuffer = PrepareUCSOutputBuffer(length); string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_dotsToChar(tables, inputBuffer, outputBuffer, input.Length, TranslationMode.Regular) > 0; + success = NativeMethods.lou_dotsToChar(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } if (!success) @@ -242,7 +244,7 @@ public string DotsToCharacters(IEnumerable tableList, string input) throw new LibLouisException($"String translation failed: {_lastLogMessage}"); } - return ConvertUCSOutputBufferToString(outputBuffer, input.Length); + return ConvertUCSOutputBufferToString(outputBuffer, length); } /// @@ -254,16 +256,18 @@ public string CharactersToDots(IEnumerable tableList, string input) { ArgumentNullException.ThrowIfNull(input, nameof(input)); + int length = CountUCSCharacters(input); + byte[] inputBuffer = PrepareUCSInputBuffer(input); - byte[] outputBuffer = PrepareUCSOutputBuffer(input.Length); - + byte[] outputBuffer = PrepareUCSOutputBuffer(length); + string tables = string.Join(',', tableList); bool success; lock (_lock) { - success = NativeMethods.lou_charToDots(tables, inputBuffer, outputBuffer, input.Length, TranslationMode.Regular) > 0; + success = NativeMethods.lou_charToDots(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } if (!success) @@ -271,8 +275,7 @@ public string CharactersToDots(IEnumerable tableList, string input) throw new LibLouisException($"String translation failed: {_lastLogMessage}"); } - return ConvertUCSOutputBufferToString(outputBuffer, input.Length); - + return ConvertUCSOutputBufferToString(outputBuffer, length); } /// @@ -323,12 +326,12 @@ public TranslatedString Translate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } - // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis - // clamps the length at the first NUL and then overwrites inlen with the number of - // characters it actually consumed, before it computes any position mapping. The - // terminator is therefore never translated and never widens a position array write. - // See InputLengthTests. - int inputLength = input.Length + 1; + // Counted in widechars, and including the NUL terminator. The terminator is safe but load + // bearing in an unobvious way: liblouis clamps the length at the first NUL and then + // overwrites inlen with the number of characters it actually consumed, before it computes + // any position mapping. It is therefore never translated and never widens a position + // array write. See InputLengthTests. + int inputLength = CountUCSCharacters(input) + 1; int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -383,12 +386,12 @@ public string Translate(IEnumerable tableList, string input, int outputL throw new ArgumentException("Spacing must be the same length as input or null"); } - // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis - // clamps the length at the first NUL and then overwrites inlen with the number of - // characters it actually consumed, before it computes any position mapping. The - // terminator is therefore never translated and never widens a position array write. - // See InputLengthTests. - int inputLength = input.Length + 1; + // Counted in widechars, and including the NUL terminator. The terminator is safe but load + // bearing in an unobvious way: liblouis clamps the length at the first NUL and then + // overwrites inlen with the number of characters it actually consumed, before it computes + // any position mapping. It is therefore never translated and never widens a position + // array write. See InputLengthTests. + int inputLength = CountUCSCharacters(input) + 1; int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -460,12 +463,12 @@ public TranslatedString BackTranslate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } - // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis - // clamps the length at the first NUL and then overwrites inlen with the number of - // characters it actually consumed, before it computes any position mapping. The - // terminator is therefore never translated and never widens a position array write. - // See InputLengthTests. - int inputLength = input.Length + 1; + // Counted in widechars, and including the NUL terminator. The terminator is safe but load + // bearing in an unobvious way: liblouis clamps the length at the first NUL and then + // overwrites inlen with the number of characters it actually consumed, before it computes + // any position mapping. It is therefore never translated and never widens a position + // array write. See InputLengthTests. + int inputLength = CountUCSCharacters(input) + 1; int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -518,12 +521,12 @@ public string BackTranslate(IEnumerable tableList, string input, int out throw new ArgumentException("Spacing must be the same length as input or null"); } - // Counts the NUL terminator, which is safe but load bearing in an unobvious way: liblouis - // clamps the length at the first NUL and then overwrites inlen with the number of - // characters it actually consumed, before it computes any position mapping. The - // terminator is therefore never translated and never widens a position array write. - // See InputLengthTests. - int inputLength = input.Length + 1; + // Counted in widechars, and including the NUL terminator. The terminator is safe but load + // bearing in an unobvious way: liblouis clamps the length at the first NUL and then + // overwrites inlen with the number of characters it actually consumed, before it computes + // any position mapping. It is therefore never translated and never widens a position + // array write. See InputLengthTests. + int inputLength = CountUCSCharacters(input) + 1; int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -627,6 +630,19 @@ public string Hyphenate(IEnumerable tableList, string input, Translation return buffer; } + /// + /// The number of liblouis widechars occupies. + /// + /// + /// Not the same as string.Length on a UCS-4 build: a non-BMP character is one widechar but + /// two chars. Lengths handed to liblouis have to be counted in widechars, or they describe a + /// longer buffer than the one that was allocated. + /// + private int CountUCSCharacters(string input) + { + return LibLouisStringEncoder.GetByteCount(input) / CharacterSize; + } + /// /// Return UCS-2/4 null terminated encoding of input. /// From f18eb7b568d0024e15bf368fbc7bd70124ced72a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 02:09:06 +0200 Subject: [PATCH 08/14] fix: Count every liblouis length in widechars Two related corrections, both about the unit lengths are measured in. Hyphenate still passed input.Length. lou_hyphenate memcpy's exactly inlen widechars with no NUL to stop at, so on a UCS-4 build - which is every binary we ship, both build scripts configure --enable-ucs4 - a word with two non-BMP characters read past the end of the input buffer, and any non-BMP character produced one flag too many. Count widechars, size the flag buffer from that, and check the HYPHSTRING limit against it too. The translate functions now pass inlen excluding the NUL terminator, which is what the header documents and what upstream callers pass. The previous input.Length + 1 was safe, but only because liblouis clamps at the first NUL and then overwrites inlen with the count it consumed - correctness rested on two undocumented internals rather than on the contract. Behaviour is unchanged; the value now equals what liblouis computed for itself anyway. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/HyphenateTests.cs | 19 +++++++++ LibLouis.NET.Test/InputLengthTests.cs | 23 ++++++----- LibLouis.NET/LibLouis.cs | 58 +++++++++++++-------------- 3 files changed, 58 insertions(+), 42 deletions(-) diff --git a/LibLouis.NET.Test/HyphenateTests.cs b/LibLouis.NET.Test/HyphenateTests.cs index 60c1946..fcc34f9 100644 --- a/LibLouis.NET.Test/HyphenateTests.cs +++ b/LibLouis.NET.Test/HyphenateTests.cs @@ -48,4 +48,23 @@ public void Hyphenate_DoesNotIncludeTheNulTerminator() Assert.Equal(word.Length, hyphens.Length); } } + + /// + /// 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. + /// + [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 = NativeShim.lou_charSize() == 4 + ? word.EnumerateRunes().Count() + : word.Length; + + string hyphens = LibLouis.Instance.Hyphenate(TablePaths(), word, TranslationMode.Regular); + + Assert.Equal(expected, hyphens.Length); + } } diff --git a/LibLouis.NET.Test/InputLengthTests.cs b/LibLouis.NET.Test/InputLengthTests.cs index 875cd06..120d509 100644 --- a/LibLouis.NET.Test/InputLengthTests.cs +++ b/LibLouis.NET.Test/InputLengthTests.cs @@ -7,19 +7,20 @@ namespace LibLouis.NET.Test; /// -/// The wrapper passes inlen as input.Length + 1, which looks like it counts the NUL terminator -/// as a character to translate. It does not, and these tests pin that down so the "+ 1" is not -/// removed - or relied on - by mistake: +/// 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: /// -/// * lou_translateString clamps the length at the first NUL -/// (while (k < *inlen && inbufx[k]) k++;, lou_translateString.c:1191), so the -/// terminator is never translated. -/// * It then overwrites *inlen with the number of characters actually consumed -/// (lou_translateString.c:1354) before computing outputPos, so the inflated value cannot -/// reach the position loops and cannot push a write past the caller's array. +/// * 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 +/// (while (k < *inlen && inbufx[k]) k++;, 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. /// -/// Both properties depend on the input buffer really being NUL terminated, which is -/// PrepareUCSInputBuffer's job. +/// 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. /// public class InputLengthTests { diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 1de6f03..4f0c438 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -326,12 +326,10 @@ public TranslatedString Translate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } - // Counted in widechars, and including the NUL terminator. The terminator is safe but load - // bearing in an unobvious way: liblouis clamps the length at the first NUL and then - // overwrites inlen with the number of characters it actually consumed, before it computes - // any position mapping. It is therefore never translated and never widens a position - // array write. See InputLengthTests. - int inputLength = CountUCSCharacters(input) + 1; + // The number of widechars to translate, excluding the NUL terminator, which is what the + // header means by inlen and what upstream callers pass. The buffer stays terminated: the + // translate functions clamp at the first NUL, so an embedded NUL still ends the input. + int inputLength = CountUCSCharacters(input); int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -386,12 +384,10 @@ public string Translate(IEnumerable tableList, string input, int outputL throw new ArgumentException("Spacing must be the same length as input or null"); } - // Counted in widechars, and including the NUL terminator. The terminator is safe but load - // bearing in an unobvious way: liblouis clamps the length at the first NUL and then - // overwrites inlen with the number of characters it actually consumed, before it computes - // any position mapping. It is therefore never translated and never widens a position - // array write. See InputLengthTests. - int inputLength = CountUCSCharacters(input) + 1; + // The number of widechars to translate, excluding the NUL terminator, which is what the + // header means by inlen and what upstream callers pass. The buffer stays terminated: the + // translate functions clamp at the first NUL, so an embedded NUL still ends the input. + int inputLength = CountUCSCharacters(input); int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -463,12 +459,10 @@ public TranslatedString BackTranslate( throw new ArgumentException($"{nameof(outputPosition)} parameter must point to an array of integers with at least input length elements.", nameof(outputPosition)); } - // Counted in widechars, and including the NUL terminator. The terminator is safe but load - // bearing in an unobvious way: liblouis clamps the length at the first NUL and then - // overwrites inlen with the number of characters it actually consumed, before it computes - // any position mapping. It is therefore never translated and never widens a position - // array write. See InputLengthTests. - int inputLength = CountUCSCharacters(input) + 1; + // The number of widechars to translate, excluding the NUL terminator, which is what the + // header means by inlen and what upstream callers pass. The buffer stays terminated: the + // translate functions clamp at the first NUL, so an embedded NUL still ends the input. + int inputLength = CountUCSCharacters(input); int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -521,12 +515,10 @@ public string BackTranslate(IEnumerable tableList, string input, int out throw new ArgumentException("Spacing must be the same length as input or null"); } - // Counted in widechars, and including the NUL terminator. The terminator is safe but load - // bearing in an unobvious way: liblouis clamps the length at the first NUL and then - // overwrites inlen with the number of characters it actually consumed, before it computes - // any position mapping. It is therefore never translated and never widens a position - // array write. See InputLengthTests. - int inputLength = CountUCSCharacters(input) + 1; + // The number of widechars to translate, excluding the NUL terminator, which is what the + // header means by inlen and what upstream callers pass. The buffer stays terminated: the + // translate functions clamp at the first NUL, so an embedded NUL still ends the input. + int inputLength = CountUCSCharacters(input); int outputBufferLength = outputLength; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -560,7 +552,8 @@ public string BackTranslate(IEnumerable tableList, string input, int out /// /// /// One character per character of : '1' where the word may be broken, - /// '0' where it may not, '2' after an existing hyphen. + /// '0' where it may not, '2' after an existing hyphen. On a UCS-4 build a non-BMP character + /// counts once, so the result can be shorter than . /// /// public string Hyphenate(IEnumerable tableList, string input, TranslationMode mode) @@ -568,9 +561,11 @@ public string Hyphenate(IEnumerable tableList, string input, Translation ArgumentNullException.ThrowIfNull(tableList); ArgumentException.ThrowIfNullOrEmpty(input); + int length = CountUCSCharacters(input); + // liblouis rejects anything from HYPHSTRING characters up, and would otherwise report it // as an ordinary hyphenation failure. - if (input.Length >= MaxHyphenationLength) + if (length >= MaxHyphenationLength) { throw new ArgumentException( $"{nameof(input)} must be shorter than {MaxHyphenationLength} characters.", nameof(input)); @@ -580,8 +575,9 @@ public string Hyphenate(IEnumerable tableList, string input, Translation // liblouis writes one flag per character plus a NUL terminator into a caller-allocated // char buffer. inlen must not count the terminator: lou_hyphenate memcpy's exactly inlen - // characters rather than stopping at a NUL the way the translate functions do. - byte[] hyphens = new byte[input.Length + 1]; + // widechars rather than stopping at a NUL the way the translate functions do, so an + // inlen in the wrong unit reads straight past the input buffer. + byte[] hyphens = new byte[length + 1]; byte[] inputBuffer = PrepareUCSInputBuffer(input); @@ -589,16 +585,16 @@ public string Hyphenate(IEnumerable tableList, string input, Translation lock (_lock) { - success = NativeMethods.lou_hyphenate(tables, inputBuffer, input.Length, hyphens, mode) > 0; + success = NativeMethods.lou_hyphenate(tables, inputBuffer, length, hyphens, mode) > 0; } - + if (!success) { throw new LibLouisException($"Hyphenation failed {_lastLogMessage}"); } // The flags are ASCII digits; the trailing terminator is not part of the result. - return Encoding.ASCII.GetString(hyphens, 0, input.Length); + return Encoding.ASCII.GetString(hyphens, 0, length); } /// From 20ffd75026644c915abe5bec299c26dd0942b36a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 06:38:25 +0200 Subject: [PATCH 09/14] fix: Serialise lou_free and refuse use after disposal Dispose called lou_free outside _lock, alone among the native calls. lou_free walks and frees the translation and display table chains, which are shared by every caller, so disposing while another thread translates is a use-after-free. With eight threads translating and a single Dispose landing mid-flight, that surfaced as LibLouisException: ... no mapping for dot pattern in display table liblouis reading a display table that had just been freed under it. A crash is equally available; this run happened to degrade into nonsense instead. Disposal also meant nothing: disposedValue was set and never read, so Instance kept handing out the object and liblouis lazily recompiled the tables that had just been thrown away. The only lasting effect was silently discarding every compiled table in the process, for every other consumer, with no error. So take the lock in Dispose, make it idempotent, and guard every native entry point with ObjectDisposedException. The guards sit inside the lock, immediately before the native call: checking on the way in would leave a window for Dispose to free the tables in between. Same run now gives eight clean ObjectDisposedExceptions and no corruption. The finalizer is gone. lou_free is process-global teardown while a finalizer runs per managed instance, so under a collectible AssemblyLoadContext it would have freed the tables of every other context still using liblouis, from the finalizer thread, outside the lock. Nothing here owns a handle that leaks if the caller never disposes. Setting a logger stays legal afterwards - it touches nothing lou_free released, and attaching a logger while shutting down is worth more than the symmetry. Whether IDisposable is the right shape at all is still open, pending the audit of the wrapper's consumers. Also pass liblouis log messages as an argument rather than as the message template, so a brace in a table path or rule is not parsed as a placeholder. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/DisposalTests.cs | 117 +++++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 68 +++++++++++++---- 2 files changed, 171 insertions(+), 14 deletions(-) create mode 100644 LibLouis.NET.Test/DisposalTests.cs diff --git a/LibLouis.NET.Test/DisposalTests.cs b/LibLouis.NET.Test/DisposalTests.cs new file mode 100644 index 0000000..3066ed4 --- /dev/null +++ b/LibLouis.NET.Test/DisposalTests.cs @@ -0,0 +1,117 @@ +using System; +using System.IO; +using System.Linq; +using System.Reflection; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// Disposing frees liblouis's translation and display table chains, which are process-global. The +/// wrapper therefore has to serialise lou_free against every other native call, and must refuse +/// to be used afterwards rather than quietly recompiling the tables it just threw away. +/// +/// +/// These tests set the disposed flag directly instead of calling Dispose. LibLouis is a +/// process-wide singleton and the suite runs serially in one process, so really disposing it +/// would fail every test that ran afterwards. The flag is restored in a finally for the same +/// reason. End-to-end disposal is exercised out of process. +/// +public class DisposalTests +{ + 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))]; + + private static FieldInfo DisposedField => + typeof(LibLouis).GetField("disposedValue", BindingFlags.NonPublic | BindingFlags.Instance) + ?? throw new InvalidOperationException("disposedValue field not found"); + + [Fact] + public void UsingTheInstanceAfterDisposeThrows() + { + WhileMarkedDisposed(() => + { + Assert.Throws( + () => LibLouis.Instance.Translate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.Translate( + TablePaths(), "abc", 16, null, null, new int[16], new int[16], 0, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.BackTranslate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc")); + + Assert.Throws( + () => LibLouis.Instance.DotsToCharacters(TablePaths(), "abc")); + + Assert.Throws( + () => LibLouis.Instance.Hyphenate(TablePaths(), "bogstaver", TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.IndexTables(TablePaths())); + + Assert.Throws( + () => LibLouis.Instance.FindTable("type:literary")); + }); + } + + /// + /// The instance is usable again once the flag is cleared, so the guard is the only thing + /// stopping it - the test is not just observing a broken singleton. + /// + [Fact] + public void TheGuardIsWhatBlocksUse() + { + WhileMarkedDisposed(() => + Assert.Throws( + () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc"))); + + Assert.Equal(3, LibLouis.Instance.CharactersToDots(TablePaths(), "abc").Length); + } + + /// + /// lou_free is process-global, but a finalizer is per managed instance. In a collectible + /// AssemblyLoadContext that would free the tables of every other context still using them, + /// and it would do it on the finalizer thread, outside the lock. + /// + [Fact] + public void LibLouisHasNoFinalizer() + { + MethodInfo? finalizer = typeof(LibLouis) + .GetMethod("Finalize", BindingFlags.NonPublic | BindingFlags.Instance); + + Assert.Equal(typeof(object), finalizer?.DeclaringType); + } + + [Fact] + public void DisposedFlagIsVolatile() + { + // Read outside the lock by the guards, written under it by Dispose. + Assert.Contains( + typeof(LibLouis).GetField("disposedValue", BindingFlags.NonPublic | BindingFlags.Instance)! + .GetRequiredCustomModifiers(), + m => m == typeof(System.Runtime.CompilerServices.IsVolatile)); + } + + private static void WhileMarkedDisposed(Action body) + { + FieldInfo field = DisposedField; + + field.SetValue(LibLouis.Instance, true); + + try + { + body(); + } + finally + { + field.SetValue(LibLouis.Instance, false); + } + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 4f0c438..9ad2a45 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -80,15 +80,17 @@ private LibLouis() NativeMethods.lou_registerLogCallback(_logCallback); } - // https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/unmanaged - ~LibLouis() - { - // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method - Dispose(disposing: false); - } + // Deliberately no finalizer. lou_free tears down state that is global to the process, while a + // finalizer runs per managed instance: in a collectible AssemblyLoadContext it would free the + // tables of every other context still using liblouis, from the finalizer thread, outside the + // lock. Nothing here owns a handle that needs releasing if the caller forgets to dispose. private ILogger _logger = NullLogger.Instance; - private bool disposedValue; + + /// + /// Read by the guards without the lock, written by under it. + /// + private volatile bool disposedValue; /// /// ILogger instance LibLouis will log to. @@ -103,6 +105,8 @@ private void SetLogger(ILogger logger) { ArgumentNullException.ThrowIfNull(logger, nameof(logger)); + // Deliberately usable after disposal: neither call touches anything lou_free released, + // and being able to attach a logger while shutting down is worth more than the symmetry. lock (_lock) { _logger = logger; @@ -132,7 +136,10 @@ private void LogCallback(LogLevel level, string message) if (_logger.IsEnabled(l)) { - _logger.Log(l, message); + // Passed as an argument, not as the template: liblouis messages contain table + // paths and rule text, and a stray brace would otherwise be parsed as a + // placeholder. + _logger.Log(l, "{LiblouisMessage}", message); } } catch @@ -163,6 +170,7 @@ public string? DataPath { lock (_lock) { + ThrowIfDisposed(); return NativeMethods.lou_getDataPath(); } } @@ -171,6 +179,7 @@ public string? DataPath ArgumentException.ThrowIfNullOrWhiteSpace(value, nameof(value)); lock (_lock) { + ThrowIfDisposed(); NativeMethods.lou_setDataPath(value); } } @@ -194,6 +203,7 @@ public string? DataPath lock (_lock) { + ThrowIfDisposed(); return NativeMethods.lou_findTable(query); } } @@ -213,6 +223,7 @@ public void IndexTables(IEnumerable tables) lock (_lock) { + ThrowIfDisposed(); NativeMethods.lou_indexTables(nullTerminated); } } @@ -236,6 +247,7 @@ public string DotsToCharacters(IEnumerable tableList, string input) lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_dotsToChar(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } @@ -267,6 +279,7 @@ public string CharactersToDots(IEnumerable tableList, string input) lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_charToDots(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } @@ -341,6 +354,7 @@ public TranslatedString Translate( lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } @@ -399,6 +413,7 @@ public string Translate(IEnumerable tableList, string input, int outputL lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_translateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } @@ -474,6 +489,7 @@ public TranslatedString BackTranslate( lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } @@ -530,6 +546,7 @@ public string BackTranslate(IEnumerable tableList, string input, int out lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_backTranslateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } @@ -585,6 +602,7 @@ public string Hyphenate(IEnumerable tableList, string input, Translation lock (_lock) { + ThrowIfDisposed(); success = NativeMethods.lou_hyphenate(tables, inputBuffer, length, hyphens, mode) > 0; } @@ -673,27 +691,49 @@ private string ConvertUCSOutputBufferToString(byte[] outputBuffer, int outputLen return LibLouisStringEncoder.GetString(outputBuffer, 0, Math.Min(outputLength * CharacterSize, outputBuffer.Length)); } + /// + /// Throws if liblouis has already been torn down. + /// + /// + /// Called from inside the lock, immediately before the native call. Checking on the way in + /// instead would leave a window for Dispose to free the tables between the check and the call. + /// + private void ThrowIfDisposed() + { + ObjectDisposedException.ThrowIf(disposedValue, this); + } + + /// + /// Frees everything liblouis has allocated. + /// + /// + /// This is process-global teardown, not the release of a per-instance resource: lou_free + /// walks and frees the translation and display table chains that every caller shares. It + /// therefore takes the same lock as every other native call - freeing those chains while + /// another thread is translating is a use-after-free, which shows up as anything from a + /// nonsense "no mapping for dot pattern" error to a crash. + /// protected virtual void Dispose(bool disposing) { - if (!disposedValue) + lock (_lock) { - if (disposing) + if (disposedValue) { - // Dispose managed state (managed objects) + return; } - // Free unmanaged resources (unmanaged objects) and override finalizer NativeMethods.lou_free(); - // Set large fields to null disposedValue = true; } } public void Dispose() { - // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method Dispose(disposing: true); + + // There is no finalizer to suppress, but a derived type could introduce one and would + // otherwise have to re-implement IDisposable just to make this call. GC.SuppressFinalize(this); } } From 533f1d2ec132a104e7b366c991b2d616d9ae97a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 06:49:46 +0200 Subject: [PATCH 10/14] fix: Serialise every native call on one shared lock Version called lou_version outside the lock, and the whole static Logging class bypassed it: SetCallback and the LogLevel setter wrote liblouis's callback pointer and log level with no synchronisation at all, while another thread could be inside a translation that reads them as it logs. The lock protects state that belongs to the native library rather than to the instance, so it is now static and shared between LibLouis and Logging. Monitor is reentrant, so a logger that calls back in while liblouis is logging still does not deadlock. lou_version returns a compile-time constant, so it was not racy in practice - but "every native call takes the lock" is worth more as a rule without exceptions than as one that has to be re-derived per function. Version and SetLogger stay usable after disposal: neither touches anything lou_free released. Holding the lock is not observable at runtime, so the regression test is source based: every NativeMethods call must sit inside a lock or carry an "unlocked:" comment giving the reason. The two type-initializer calls are the only exemptions. Verified the test fails when a lock is removed, rather than passing vacuously. Also clean up warnings in the test project: the raw shim is now SafeNativeMethods with explicit search paths and pre-encoded UTF-8 arguments, and the concurrency test awaits instead of blocking. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/HyphenateTests.cs | 2 +- LibLouis.NET.Test/NativeLockTests.cs | 196 +++++++++++++++++++++++ LibLouis.NET.Test/NativeShim.cs | 26 --- LibLouis.NET.Test/NonBmpTests.cs | 2 +- LibLouis.NET.Test/SafeNativeMethods.cs | 41 +++++ LibLouis.NET.Test/TypeFormBufferTests.cs | 13 +- LibLouis.NET/LibLouis.cs | 51 +++--- LibLouis.NET/Logging.cs | 24 ++- 8 files changed, 301 insertions(+), 54 deletions(-) create mode 100644 LibLouis.NET.Test/NativeLockTests.cs delete mode 100644 LibLouis.NET.Test/NativeShim.cs create mode 100644 LibLouis.NET.Test/SafeNativeMethods.cs diff --git a/LibLouis.NET.Test/HyphenateTests.cs b/LibLouis.NET.Test/HyphenateTests.cs index fcc34f9..a9b022e 100644 --- a/LibLouis.NET.Test/HyphenateTests.cs +++ b/LibLouis.NET.Test/HyphenateTests.cs @@ -59,7 +59,7 @@ public void Hyphenate_DoesNotIncludeTheNulTerminator() [InlineData("bogstaver\U0001D11E\U0001D11E")] // and reads past the input buffer public void Hyphenate_ReturnsOneFlagPerWidecharNotPerCodeUnit(string word) { - int expected = NativeShim.lou_charSize() == 4 + int expected = SafeNativeMethods.lou_charSize() == 4 ? word.EnumerateRunes().Count() : word.Length; diff --git a/LibLouis.NET.Test/NativeLockTests.cs b/LibLouis.NET.Test/NativeLockTests.cs new file mode 100644 index 0000000..d36966c --- /dev/null +++ b/LibLouis.NET.Test/NativeLockTests.cs @@ -0,0 +1,196 @@ +using System; +using System.Collections.Concurrent; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// liblouis is not thread safe and its state is process-global, so every native call in the +/// assembly has to serialise on one lock - including the ones that do not obviously touch shared +/// state. +/// +/// +/// Holding the lock is not directly observable: lou_version returns a static string and the +/// Logging setters are single pointer-sized writes, so an unsynchronised build does not reliably +/// misbehave. These tests therefore guard the two things that are observable - that no native +/// entry point was left outside the lock, and that adding the lock did not introduce a deadlock +/// or change behaviour. +/// +public class NativeLockTests +{ + 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 VersionIsReported() + { + Assert.False(string.IsNullOrWhiteSpace(LibLouis.Instance.Version)); + } + + [Fact] + public void LogLevelRoundTrips() + { + LogLevel previous = Logging.LogLevel; + + try + { + Logging.LogLevel = LogLevel.Warning; + Assert.Equal(LogLevel.Warning, Logging.LogLevel); + } + finally + { + Logging.LogLevel = previous; + } + } + + /// + /// The lock is shared between LibLouis and the static Logging helper, and Monitor is + /// reentrant, so hammering all three from several threads must neither deadlock nor produce a + /// wrong translation. + /// + [Fact] + public async Task ConcurrentUseDoesNotDeadlockOrCorrupt() + { + const string input = "Første linje"; + const string expected = "@fze linje"; + + LogLevel previous = Logging.LogLevel; + + using CancellationTokenSource cts = new(TimeSpan.FromSeconds(5)); + + ConcurrentBag failures = []; + + try + { + Task[] workers = + [ + .. Enumerable.Range(0, 4).Select(_ => Task.Run(() => + { + while (!cts.IsCancellationRequested) + { + string result = LibLouis.Instance.Translate( + TablePaths(), input, 64, null, null, TranslationMode.Regular); + + if (result != expected) + { + failures.Add($"translation returned '{result}'"); + return; + } + } + })), + Task.Run(() => + { + while (!cts.IsCancellationRequested) + { + _ = LibLouis.Instance.Version; + Logging.LogLevel = LogLevel.Error; + } + }), + ]; + + Task all = Task.WhenAll(workers); + + Assert.Same( + all, + await Task.WhenAny(all, Task.Delay(TimeSpan.FromSeconds(30)))); + + await all; + } + finally + { + Logging.LogLevel = previous; + } + + Assert.Empty(failures); + } + + /// + /// Catches a native call added later without the lock. Deliberately source-based: there is no + /// runtime signal for "this P/Invoke ran unsynchronised". + /// + /// + /// A call that genuinely does not need the lock has to say so, by carrying an "unlocked:" + /// comment giving the reason. That keeps the exemptions few and explains each one, instead of + /// letting the test quietly special-case whole methods. + /// + [Theory] + [InlineData("LibLouis.cs")] + [InlineData("Logging.cs")] + public void EveryNativeCallSiteIsLockedOrJustified(string fileName) + { + string[] lines = ReadLibrarySource(fileName).Split('\n'); + + int depth = 0; + int lockDepth = -1; + bool pendingLock = false; + + for (int i = 0; i < lines.Length; i++) + { + string line = lines[i].Trim(); + + // The body starts at the brace on the next line, so record the depth once we are + // actually inside it rather than on the "lock (" line itself. + if (line.StartsWith("lock (", StringComparison.Ordinal)) + { + pendingLock = true; + } + + bool isNativeCall = line.Contains("NativeMethods.", StringComparison.Ordinal) + && !line.StartsWith("//", StringComparison.Ordinal) + && !line.StartsWith("///", StringComparison.Ordinal) + && !line.Contains("NativeMethods.LoggingCallback", StringComparison.Ordinal); + + if (isNativeCall && lockDepth < 0) + { + bool justified = lines + .Take(i) + .Reverse() + .TakeWhile(l => l.Trim().StartsWith("//", StringComparison.Ordinal)) + .Any(l => l.Contains("unlocked:", StringComparison.Ordinal)); + + Assert.True(justified, $"{fileName}: native call outside a lock: {line}"); + } + + int updated = depth + lines[i].Count(c => c == '{') - lines[i].Count(c => c == '}'); + + if (pendingLock && updated > depth) + { + lockDepth = updated; + pendingLock = false; + } + + depth = updated; + + if (lockDepth >= 0 && depth < lockDepth) + { + lockDepth = -1; + } + } + } + + 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"))) + { + directory = directory.Parent; + } + + Assert.NotNull(directory); + + string path = Path.Combine(directory.FullName, "LibLouis.NET", fileName); + + Assert.True(File.Exists(path), $"could not locate {path}"); + + return File.ReadAllText(path); + } +} diff --git a/LibLouis.NET.Test/NativeShim.cs b/LibLouis.NET.Test/NativeShim.cs deleted file mode 100644 index 575cede..0000000 --- a/LibLouis.NET.Test/NativeShim.cs +++ /dev/null @@ -1,26 +0,0 @@ -using System.Runtime.InteropServices; - -namespace LibLouis.NET.Test; - -/// -/// Raw P/Invoke used to characterise native behaviour without going through the wrapper. -/// -internal static class NativeShim -{ - /// - /// Bytes per liblouis widechar: 2 for a UCS-2 build, 4 for UCS-4. - /// - [DllImport("liblouis", EntryPoint = "lou_charSize")] - internal static extern int lou_charSize(); - - [DllImport("liblouis", EntryPoint = "lou_translateString")] - internal static extern int lou_translateString( - [MarshalAs(UnmanagedType.LPUTF8Str)] string tableList, - byte[] inbuf, - ref int inlen, - byte[] outbuf, - ref int outlen, - ushort[]? typeform, - [MarshalAs(UnmanagedType.LPUTF8Str)] string? spacing, - int mode); -} diff --git a/LibLouis.NET.Test/NonBmpTests.cs b/LibLouis.NET.Test/NonBmpTests.cs index 241147e..261417e 100644 --- a/LibLouis.NET.Test/NonBmpTests.cs +++ b/LibLouis.NET.Test/NonBmpTests.cs @@ -31,7 +31,7 @@ private static string[] TablePaths() => /// private static int ExpectedCells(string value) { - return NativeShim.lou_charSize() == 4 + return SafeNativeMethods.lou_charSize() == 4 ? value.EnumerateRunes().Count() : value.Length; } diff --git a/LibLouis.NET.Test/SafeNativeMethods.cs b/LibLouis.NET.Test/SafeNativeMethods.cs new file mode 100644 index 0000000..0bcf339 --- /dev/null +++ b/LibLouis.NET.Test/SafeNativeMethods.cs @@ -0,0 +1,41 @@ +using System.Runtime.InteropServices; +using System.Text; + +namespace LibLouis.NET.Test; + +/// +/// Raw P/Invoke used to characterise native behaviour without going through the wrapper. +/// +/// +/// Strings are passed as pre-encoded NUL terminated UTF-8 rather than as managed strings, so +/// there is no marshalling behaviour of our own between the test and liblouis. +/// +internal static class SafeNativeMethods +{ + /// + /// Bytes per liblouis widechar: 2 for a UCS-2 build, 4 for UCS-4. + /// + [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] + [DllImport("liblouis", EntryPoint = "lou_charSize")] + internal static extern int lou_charSize(); + + [DefaultDllImportSearchPaths(DllImportSearchPath.SafeDirectories)] + [DllImport("liblouis", EntryPoint = "lou_translateString")] + internal static extern int lou_translateString( + byte[] tableList, + byte[] inbuf, + ref int inlen, + byte[] outbuf, + ref int outlen, + ushort[]? typeform, + byte[]? spacing, + int mode); + + /// + /// Encodes a string the way liblouis expects a const char *. + /// + internal static byte[] Utf8(string value) + { + return Encoding.UTF8.GetBytes(value + "\0"); + } +} diff --git a/LibLouis.NET.Test/TypeFormBufferTests.cs b/LibLouis.NET.Test/TypeFormBufferTests.cs index 721a0a2..c4fd76d 100644 --- a/LibLouis.NET.Test/TypeFormBufferTests.cs +++ b/LibLouis.NET.Test/TypeFormBufferTests.cs @@ -34,7 +34,7 @@ private static string[] TablePaths() => [Fact] public void Native_WritesOneTypeformEntryPerOutputCell() { - int charSize = NativeShim.lou_charSize(); + int charSize = SafeNativeMethods.lou_charSize(); Encoding encoder = charSize == 4 ? Encoding.UTF32 : Encoding.Unicode; int inputLength = Input.Length; @@ -53,8 +53,15 @@ public void Native_WritesOneTypeformEntryPerOutputCell() int inLen = inputLength; int outLen = outputLength; - int result = NativeShim.lou_translateString( - string.Join(',', TablePaths()), inputBuffer, ref inLen, outputBuffer, ref outLen, typeform, null, 0); + int result = SafeNativeMethods.lou_translateString( + SafeNativeMethods.Utf8(string.Join(',', TablePaths())), + inputBuffer, + ref inLen, + outputBuffer, + ref outLen, + typeform, + null, + 0); Assert.NotEqual(0, result); Assert.Equal(ExpectedOutput, encoder.GetString(outputBuffer, 0, outLen * charSize)); diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 9ad2a45..45d47f1 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -27,7 +27,13 @@ public class LibLouis : IDisposable /// /// LibLouis is *NOT* thread safe, so we'll have to use a lock to avoid concurrrent access to native liblouis calls. /// - private readonly object _lock; + /// + /// Static, and shared with : the state it protects belongs to the native + /// library, not to this instance, so every native call in the assembly has to serialise on the + /// same object. Monitor is reentrant, so a logger that calls back in while liblouis is logging + /// does not deadlock. + /// + internal static readonly object NativeLock = new(); /// /// LibLouis can currently use either UCS-4 (1:1 mapping of UTF-32), or UCS-2 (WTF-16 without surrogate pairs), @@ -66,7 +72,8 @@ static LibLouis() private LibLouis() { - _lock = new object(); + // unlocked: the type initializer runs single threaded, and no other thread can hold a + // reference to the singleton until it has finished, so there is nothing to race with. CharacterSize = NativeMethods.lou_charSize(); LibLouisStringEncoder = CharacterSize switch { @@ -75,8 +82,10 @@ private LibLouis() _ => throw new NotImplementedException($"Liblouis is a character size of {CharacterSize}!?"), }; - // Register managed log callback, so we can give reasonable exception messages. _logCallback = LogCallback; + + // Register managed log callback, so we can give reasonable exception messages. + // unlocked: same reason - still inside the type initializer. NativeMethods.lou_registerLogCallback(_logCallback); } @@ -107,7 +116,7 @@ private void SetLogger(ILogger logger) // Deliberately usable after disposal: neither call touches anything lou_free released, // and being able to attach a logger while shutting down is worth more than the symmetry. - lock (_lock) + lock (NativeLock) { _logger = logger; NativeMethods.lou_setLogLevel(LogLevel.All); @@ -151,12 +160,18 @@ private void LogCallback(LogLevel level, string message) /// /// Returns version number of the native liblouis library. /// + /// + /// Readable after disposal: lou_version returns a compile-time constant and touches nothing + /// lou_free released, and version information is worth having while diagnosing a shutdown. + /// public string Version { get { - string version = NativeMethods.lou_version(); - return version; + lock (NativeLock) + { + return NativeMethods.lou_version(); + } } } @@ -168,7 +183,7 @@ public string? DataPath { get { - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); return NativeMethods.lou_getDataPath(); @@ -177,7 +192,7 @@ public string? DataPath set { ArgumentException.ThrowIfNullOrWhiteSpace(value, nameof(value)); - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); NativeMethods.lou_setDataPath(value); @@ -201,7 +216,7 @@ public string? DataPath { ArgumentException.ThrowIfNullOrWhiteSpace(query, nameof(query)); - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); return NativeMethods.lou_findTable(query); @@ -221,7 +236,7 @@ public void IndexTables(IEnumerable tables) // hands it to _lou_logMessage as a string. string?[] nullTerminated = [.. tables, null]; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); NativeMethods.lou_indexTables(nullTerminated); @@ -245,7 +260,7 @@ public string DotsToCharacters(IEnumerable tableList, string input) string tables = string.Join(',', tableList); bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_dotsToChar(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; @@ -277,7 +292,7 @@ public string CharactersToDots(IEnumerable tableList, string input) bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_charToDots(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; @@ -352,7 +367,7 @@ public TranslatedString Translate( string tables = string.Join(',', tableList); bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; @@ -411,7 +426,7 @@ public string Translate(IEnumerable tableList, string input, int outputL string tables = string.Join(',', tableList); bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_translateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; @@ -487,7 +502,7 @@ public TranslatedString BackTranslate( string tables = string.Join(',', tableList); bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; @@ -544,7 +559,7 @@ public string BackTranslate(IEnumerable tableList, string input, int out string tables = string.Join(',', tableList); bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_backTranslateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; @@ -600,7 +615,7 @@ public string Hyphenate(IEnumerable tableList, string input, Translation bool success; - lock (_lock) + lock (NativeLock) { ThrowIfDisposed(); success = NativeMethods.lou_hyphenate(tables, inputBuffer, length, hyphens, mode) > 0; @@ -715,7 +730,7 @@ private void ThrowIfDisposed() /// protected virtual void Dispose(bool disposing) { - lock (_lock) + lock (NativeLock) { if (disposedValue) { diff --git a/LibLouis.NET/Logging.cs b/LibLouis.NET/Logging.cs index 4114e00..e3449e1 100644 --- a/LibLouis.NET/Logging.cs +++ b/LibLouis.NET/Logging.cs @@ -2,6 +2,11 @@ namespace LibLouis.NET; +/// +/// These change the same global liblouis state that uses, so they take the +/// same lock. Setting the callback or the log level while another thread is inside a translation +/// is otherwise an unsynchronised write to state liblouis reads as it logs. +/// public static class Logging { /// @@ -14,8 +19,11 @@ public static void SetCallback(NativeMethods.LoggingCallback value) { ArgumentNullException.ThrowIfNull(value); - _callback = value; - NativeMethods.lou_registerLogCallback(_callback); + lock (LibLouis.NativeLock) + { + _callback = value; + NativeMethods.lou_registerLogCallback(_callback); + } } private static LogLevel _logLevel = LogLevel.Off; @@ -24,12 +32,18 @@ public static LogLevel LogLevel { get { - return _logLevel; + lock (LibLouis.NativeLock) + { + return _logLevel; + } } set { - _logLevel = value; - NativeMethods.lou_setLogLevel(value); + lock (LibLouis.NativeLock) + { + _logLevel = value; + NativeMethods.lou_setLogLevel(value); + } } } From 7500054245200d8df4434cd9db5742c578dfa994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 07:00:55 +0200 Subject: [PATCH 11/14] refactor!: Replace IDisposable with a static Shutdown() BREAKING: LibLouis no longer implements IDisposable. Replace LibLouis.Instance.Dispose() with LibLouis.Shutdown(), and delete any using statement over the singleton - those now fail to compile, which is the point. IDisposable promises something this type cannot honour. It says "I own a resource, dispose me when you are done", but Instance is a process-wide singleton over process-global native state: no caller is ever done with it, and lou_free is teardown for the whole application rather than the release of anything one caller owns. The cost was a footgun that read as good practice. This compiled: using var louis = LibLouis.Instance; and left every other consumer in the process unable to translate. CA2000 and IDE0063 actively suggest writing it. A static Shutdown() is not called by accident, no analyzer proposes it, and the name says what it does. Its one legitimate use is releasing table memory before the process exits, for leak checking. Normal applications should not call it: liblouis caches compiled tables per table list rather than per call, so nothing accumulates, and process exit reclaims it anyway. The guard throws InvalidOperationException rather than ObjectDisposedException. "Cannot access a disposed object" would send a reader looking for a Dispose call that no longer exists; the message now says liblouis was shut down and that it cannot be undone. Version and the Logger setter still work afterwards - neither touches anything lou_free released, and diagnostics are worth most during shutdown. Verified out of process: 6562 translations, Shutdown mid-flight, all eight workers refused cleanly with zero corruption, a second Shutdown is a no-op, and "using var louis = LibLouis.Instance" now fails with CS1674. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/DisposalTests.cs | 117 -------------------- LibLouis.NET.Test/ShutdownTests.cs | 170 +++++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 85 +++++++++------ 3 files changed, 219 insertions(+), 153 deletions(-) delete mode 100644 LibLouis.NET.Test/DisposalTests.cs create mode 100644 LibLouis.NET.Test/ShutdownTests.cs diff --git a/LibLouis.NET.Test/DisposalTests.cs b/LibLouis.NET.Test/DisposalTests.cs deleted file mode 100644 index 3066ed4..0000000 --- a/LibLouis.NET.Test/DisposalTests.cs +++ /dev/null @@ -1,117 +0,0 @@ -using System; -using System.IO; -using System.Linq; -using System.Reflection; - -using Xunit; - -namespace LibLouis.NET.Test; - -/// -/// Disposing frees liblouis's translation and display table chains, which are process-global. The -/// wrapper therefore has to serialise lou_free against every other native call, and must refuse -/// to be used afterwards rather than quietly recompiling the tables it just threw away. -/// -/// -/// These tests set the disposed flag directly instead of calling Dispose. LibLouis is a -/// process-wide singleton and the suite runs serially in one process, so really disposing it -/// would fail every test that ran afterwards. The flag is restored in a finally for the same -/// reason. End-to-end disposal is exercised out of process. -/// -public class DisposalTests -{ - 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))]; - - private static FieldInfo DisposedField => - typeof(LibLouis).GetField("disposedValue", BindingFlags.NonPublic | BindingFlags.Instance) - ?? throw new InvalidOperationException("disposedValue field not found"); - - [Fact] - public void UsingTheInstanceAfterDisposeThrows() - { - WhileMarkedDisposed(() => - { - Assert.Throws( - () => LibLouis.Instance.Translate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); - - Assert.Throws( - () => LibLouis.Instance.Translate( - TablePaths(), "abc", 16, null, null, new int[16], new int[16], 0, TranslationMode.Regular)); - - Assert.Throws( - () => LibLouis.Instance.BackTranslate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); - - Assert.Throws( - () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc")); - - Assert.Throws( - () => LibLouis.Instance.DotsToCharacters(TablePaths(), "abc")); - - Assert.Throws( - () => LibLouis.Instance.Hyphenate(TablePaths(), "bogstaver", TranslationMode.Regular)); - - Assert.Throws( - () => LibLouis.Instance.IndexTables(TablePaths())); - - Assert.Throws( - () => LibLouis.Instance.FindTable("type:literary")); - }); - } - - /// - /// The instance is usable again once the flag is cleared, so the guard is the only thing - /// stopping it - the test is not just observing a broken singleton. - /// - [Fact] - public void TheGuardIsWhatBlocksUse() - { - WhileMarkedDisposed(() => - Assert.Throws( - () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc"))); - - Assert.Equal(3, LibLouis.Instance.CharactersToDots(TablePaths(), "abc").Length); - } - - /// - /// lou_free is process-global, but a finalizer is per managed instance. In a collectible - /// AssemblyLoadContext that would free the tables of every other context still using them, - /// and it would do it on the finalizer thread, outside the lock. - /// - [Fact] - public void LibLouisHasNoFinalizer() - { - MethodInfo? finalizer = typeof(LibLouis) - .GetMethod("Finalize", BindingFlags.NonPublic | BindingFlags.Instance); - - Assert.Equal(typeof(object), finalizer?.DeclaringType); - } - - [Fact] - public void DisposedFlagIsVolatile() - { - // Read outside the lock by the guards, written under it by Dispose. - Assert.Contains( - typeof(LibLouis).GetField("disposedValue", BindingFlags.NonPublic | BindingFlags.Instance)! - .GetRequiredCustomModifiers(), - m => m == typeof(System.Runtime.CompilerServices.IsVolatile)); - } - - private static void WhileMarkedDisposed(Action body) - { - FieldInfo field = DisposedField; - - field.SetValue(LibLouis.Instance, true); - - try - { - body(); - } - finally - { - field.SetValue(LibLouis.Instance, false); - } - } -} diff --git a/LibLouis.NET.Test/ShutdownTests.cs b/LibLouis.NET.Test/ShutdownTests.cs new file mode 100644 index 0000000..e41c0fc --- /dev/null +++ b/LibLouis.NET.Test/ShutdownTests.cs @@ -0,0 +1,170 @@ +using System; +using System.IO; +using System.Linq; +using System.Reflection; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// Shutdown frees liblouis's translation and display table chains, which are global to the +/// process. It is deliberately not IDisposable: nothing here is owned by a single caller, so +/// there is no "done with it" moment to hang disposal off, and an accidental +/// using (LibLouis.Instance) would tear liblouis down for everything else in the process. +/// +/// +/// These tests set the flag directly instead of calling Shutdown. LibLouis is a process-wide +/// singleton and the suite runs serially in one process, so really shutting it down would fail +/// every test that ran afterwards. The flag is restored in a finally for the same reason. +/// End-to-end shutdown is exercised out of process. +/// +public class ShutdownTests +{ + 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))]; + + private static FieldInfo ShutDownField => + typeof(LibLouis).GetField("_shutDown", BindingFlags.NonPublic | BindingFlags.Static) + ?? throw new InvalidOperationException("_shutDown field not found"); + + /// + /// The shape change itself: an accidental using statement must not compile. + /// + [Fact] + public void LibLouisIsNotDisposable() + { + Assert.False(typeof(IDisposable).IsAssignableFrom(typeof(LibLouis))); + } + + /// + /// Shutdown is process-wide teardown, so it belongs on the type, not on an instance nobody + /// exclusively owns. + /// + [Fact] + public void ShutdownIsStatic() + { + MethodInfo? shutdown = typeof(LibLouis).GetMethod( + "Shutdown", BindingFlags.Public | BindingFlags.Static, Type.EmptyTypes); + + Assert.NotNull(shutdown); + Assert.Equal(typeof(void), shutdown.ReturnType); + } + + [Fact] + public void UsingTheInstanceAfterShutdownThrows() + { + WhileMarkedShutDown(() => + { + Assert.Throws( + () => LibLouis.Instance.Translate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.Translate( + TablePaths(), "abc", 16, null, null, new int[16], new int[16], 0, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.BackTranslate(TablePaths(), "abc", 16, null, null, TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc")); + + Assert.Throws( + () => LibLouis.Instance.DotsToCharacters(TablePaths(), "abc")); + + Assert.Throws( + () => LibLouis.Instance.Hyphenate(TablePaths(), "bogstaver", TranslationMode.Regular)); + + Assert.Throws( + () => LibLouis.Instance.IndexTables(TablePaths())); + + Assert.Throws( + () => LibLouis.Instance.FindTable("type:literary")); + }); + } + + /// + /// The message has to say what happened: "cannot access a disposed object" would be a lie for + /// a type that is not disposable. + /// + [Fact] + public void TheFailureExplainsItself() + { + WhileMarkedShutDown(() => + { + InvalidOperationException e = Assert.Throws( + () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc")); + + Assert.Contains("shut down", e.Message, StringComparison.OrdinalIgnoreCase); + }); + } + + /// + /// Diagnostics stay available: neither touches anything lou_free released. + /// + [Fact] + public void VersionAndLoggerSurviveShutdown() + { + WhileMarkedShutDown(() => + { + Assert.False(string.IsNullOrWhiteSpace(LibLouis.Instance.Version)); + + LibLouis.Instance.Logger = new CollectingLogger(); + }); + } + + /// + /// The instance works again once the flag is cleared, so the guard is the only thing stopping + /// it - the test is not just observing a broken singleton. + /// + [Fact] + public void TheGuardIsWhatBlocksUse() + { + WhileMarkedShutDown(() => + Assert.Throws( + () => LibLouis.Instance.CharactersToDots(TablePaths(), "abc"))); + + Assert.Equal(3, LibLouis.Instance.CharactersToDots(TablePaths(), "abc").Length); + } + + /// + /// lou_free is process-global, but a finalizer is per managed instance. In a collectible + /// AssemblyLoadContext that would free the tables of every other context still using them, + /// and it would do it on the finalizer thread, outside the lock. + /// + [Fact] + public void LibLouisHasNoFinalizer() + { + MethodInfo? finalizer = typeof(LibLouis) + .GetMethod("Finalize", BindingFlags.NonPublic | BindingFlags.Instance); + + Assert.Equal(typeof(object), finalizer?.DeclaringType); + } + + [Fact] + public void ShutDownFlagIsVolatile() + { + // Read outside the lock by the guards, written under it by Shutdown. + Assert.Contains( + ShutDownField.GetRequiredCustomModifiers(), + m => m == typeof(System.Runtime.CompilerServices.IsVolatile)); + } + + private static void WhileMarkedShutDown(Action body) + { + FieldInfo field = ShutDownField; + + field.SetValue(null, true); + + try + { + body(); + } + finally + { + field.SetValue(null, false); + } + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index 45d47f1..d5cef67 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -8,7 +8,7 @@ namespace LibLouis.NET; -public class LibLouis : IDisposable +public class LibLouis { /// /// LibLouis loglevels to ILogger logLevels table. @@ -92,14 +92,17 @@ private LibLouis() // Deliberately no finalizer. lou_free tears down state that is global to the process, while a // finalizer runs per managed instance: in a collectible AssemblyLoadContext it would free the // tables of every other context still using liblouis, from the finalizer thread, outside the - // lock. Nothing here owns a handle that needs releasing if the caller forgets to dispose. + // lock. Nothing here owns a handle that leaks if Shutdown is never called. private ILogger _logger = NullLogger.Instance; /// - /// Read by the guards without the lock, written by under it. + /// Read by the guards without the lock, written by under it. /// - private volatile bool disposedValue; + /// + /// Static because what it tracks is the state of the native library, not of this instance. + /// + private static volatile bool _shutDown; /// /// ILogger instance LibLouis will log to. @@ -185,7 +188,7 @@ public string? DataPath { lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); return NativeMethods.lou_getDataPath(); } } @@ -194,7 +197,7 @@ public string? DataPath ArgumentException.ThrowIfNullOrWhiteSpace(value, nameof(value)); lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); NativeMethods.lou_setDataPath(value); } } @@ -218,7 +221,7 @@ public string? DataPath lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); return NativeMethods.lou_findTable(query); } } @@ -238,7 +241,7 @@ public void IndexTables(IEnumerable tables) lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); NativeMethods.lou_indexTables(nullTerminated); } } @@ -262,7 +265,7 @@ public string DotsToCharacters(IEnumerable tableList, string input) lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_dotsToChar(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } @@ -294,7 +297,7 @@ public string CharactersToDots(IEnumerable tableList, string input) lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_charToDots(tables, inputBuffer, outputBuffer, length, TranslationMode.Regular) > 0; } @@ -369,7 +372,7 @@ public TranslatedString Translate( lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } @@ -428,7 +431,7 @@ public string Translate(IEnumerable tableList, string input, int outputL lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_translateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } @@ -504,7 +507,7 @@ public TranslatedString BackTranslate( lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; } @@ -561,7 +564,7 @@ public string BackTranslate(IEnumerable tableList, string input, int out lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_backTranslateString(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, mode) > 0; } @@ -617,7 +620,7 @@ public string Hyphenate(IEnumerable tableList, string input, Translation lock (NativeLock) { - ThrowIfDisposed(); + ThrowIfShutDown(); success = NativeMethods.lou_hyphenate(tables, inputBuffer, length, hyphens, mode) > 0; } @@ -711,44 +714,54 @@ private string ConvertUCSOutputBufferToString(byte[] outputBuffer, int outputLen /// /// /// Called from inside the lock, immediately before the native call. Checking on the way in - /// instead would leave a window for Dispose to free the tables between the check and the call. + /// instead would leave a window for Shutdown to free the tables between check and call. + /// + /// Not ObjectDisposedException: this type is not disposable, and "cannot access a disposed + /// object" would send the reader looking for a Dispose call that does not exist. /// - private void ThrowIfDisposed() + private static void ThrowIfShutDown() { - ObjectDisposedException.ThrowIf(disposedValue, this); + if (_shutDown) + { + throw new InvalidOperationException( + "liblouis has been shut down. LibLouis.Shutdown() frees state that is global to " + + "the process and cannot be undone."); + } } /// - /// Frees everything liblouis has allocated. + /// Frees everything liblouis has allocated. Final: there is no way back. /// /// - /// This is process-global teardown, not the release of a per-instance resource: lou_free - /// walks and frees the translation and display table chains that every caller shares. It - /// therefore takes the same lock as every other native call - freeing those chains while - /// another thread is translating is a use-after-free, which shows up as anything from a - /// nonsense "no mapping for dot pattern" error to a crash. + /// Deliberately a static method rather than IDisposable. lou_free walks and frees the + /// translation and display table chains, which are global to the process, so this is teardown + /// for the whole application rather than the release of a resource one caller owns. Exposing + /// it as IDisposable invited using (LibLouis.Instance), which reads as ordinary + /// cleanup and would leave every other consumer in the process unable to translate. + /// + /// Only worth calling when you need the tables released before the process exits - checking + /// for leaks, say. Normal applications should not call it at all: liblouis caches compiled + /// tables per table list rather than per call, so nothing accumulates, and process exit + /// reclaims it anyway. + /// + /// Takes the same lock as every other native call. Freeing those chains while another thread + /// is translating is a use-after-free, which shows up as anything from a nonsense + /// "no mapping for dot pattern" error to a crash. + /// + /// Calling it more than once does nothing. /// - protected virtual void Dispose(bool disposing) + public static void Shutdown() { lock (NativeLock) { - if (disposedValue) + if (_shutDown) { return; } NativeMethods.lou_free(); - disposedValue = true; + _shutDown = true; } } - - public void Dispose() - { - Dispose(disposing: true); - - // There is no finalizer to suppress, but a derived type could introduce one and would - // otherwise have to re-implement IDisposable just to make this call. - GC.SuppressFinalize(this); - } } From 3353fe7b07a9b4c7afc43ceb114f9a7e9e921d66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 07:26:50 +0200 Subject: [PATCH 12/14] feat: Surface liblouis's dot 7/8 report on TranslatedString The typeform overrun fix routed liblouis's write-back into an internal scratch buffer and then discarded it, which silently dropped the one piece of output the parameter produces: per output cell, whether the cell contains dot 7 or dot 8 (lou_translateString.c:1330). Consumers want that information. Exposing it does not reopen the memory-safety problem, because the danger was never the data - it was where liblouis wrote it. The scratch buffer is output-sized, so the write-back lands safely there; the wrapper now copies it out as TranslatedString.OutputDots78, a bool per output cell. The caller's input-sized formtype array stays untouched. Booleans rather than the raw buffer because the values are the ASCII characters '0' and '8' smuggled through a formtype array, not TypeForm flags - handing those out as TypeForm would invite bitwise tests that can never be true. Null when no formtype array was passed: liblouis only computes the information when one is supplied, and an all-false array would be indistinguishable from a real report of "no dots 7/8 anywhere". Forward translation only. Back-translation zero-fills the buffer and reports nothing (lou_backTranslateString.c:228). Verified against da-dk-g08.ctb, where a capital is marked with dot 7 on its own cell: "Abc" reports the flag on cell 0 and nowhere else. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/OutputDotsTests.cs | 100 +++++++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 28 ++++++++ LibLouis.NET/TranslatedString.cs | 19 ++++- 3 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 LibLouis.NET.Test/OutputDotsTests.cs diff --git a/LibLouis.NET.Test/OutputDotsTests.cs b/LibLouis.NET.Test/OutputDotsTests.cs new file mode 100644 index 0000000..3835a63 --- /dev/null +++ b/LibLouis.NET.Test/OutputDotsTests.cs @@ -0,0 +1,100 @@ +using System; +using System.IO; +using System.Linq; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// On a successful forward translation liblouis reports, per output cell, whether the cell +/// contains dot 7 or dot 8 (lou_translateString.c:1330). It writes that into the typeform +/// buffer - which is why the buffer has to be output-sized, and why the caller's input-sized +/// array must not receive it. The wrapper surfaces the information on TranslatedString instead, +/// so callers get it without the write-past-the-end hazard. +/// +public class OutputDotsTests +{ + private static string[] EightDotTables() => + [.. new[] { "da-dk-braillo.dis", "da-dk-g08.ctb" } + .Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))]; + + private static TranslatedString TranslateWithTypeForm(string input) + { + TypeForm[] typeform = new TypeForm[input.Length]; + + return LibLouis.Instance.Translate( + EightDotTables(), + input, + input.Length * 4, + typeform, + null, + new int[input.Length], + new int[input.Length * 4], + 0, + TranslationMode.Regular); + } + + /// + /// Danish 8-dot braille marks a capital with dot 7 on the letter's own cell, so casing gives + /// a per-cell pattern we can predict: the flag must differ between the capital and the small + /// letters. + /// + [Fact] + public void ReportsDot7OnCapitalCells() + { + TranslatedString result = TranslateWithTypeForm("Abc"); + + Assert.NotNull(result.OutputDots78); + Assert.Equal(result.Output.Length, result.OutputDots78.Length); + + Assert.True(result.OutputDots78[0], "capital A should carry dot 7 in an 8-dot table"); + Assert.All(result.OutputDots78.Skip(1), d => Assert.False(d, "small letters should not")); + } + + /// + /// liblouis only computes the information when a typeform buffer is supplied, so without one + /// the property must be null rather than a fabricated all-false array. + /// + [Fact] + public void IsNullWhenNoTypeFormWasPassed() + { + TranslatedString result = LibLouis.Instance.Translate( + EightDotTables(), + "Abc", + 16, + null, + null, + new int[3], + new int[16], + 0, + TranslationMode.Regular); + + Assert.Null(result.OutputDots78); + } + + /// + /// The safety half of the contract, restated from the caller's side: surfacing the output + /// information must not bring back the write-back into the caller's array. + /// + [Fact] + public void CallersArrayStaysUntouched() + { + TypeForm[] typeform = new TypeForm[3]; + Array.Fill(typeform, TypeForm.Italic); + + TranslatedString result = LibLouis.Instance.Translate( + EightDotTables(), + "Abc", + 16, + typeform, + null, + new int[3], + new int[16], + 0, + TranslationMode.Regular); + + Assert.NotNull(result.OutputDots78); + Assert.All(typeform, t => Assert.Equal(TypeForm.Italic, t)); + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index d5cef67..be4d22b 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -387,6 +387,7 @@ public TranslatedString Translate( CursorPosition = cursorPosition, InputPosition = inputPosition, OutputPosition = outputPosition, + OutputDots78 = ExtractOutputDots78(typeFormBuffer, outputLength), }; } @@ -662,6 +663,33 @@ public string Hyphenate(IEnumerable tableList, string input, Translation return buffer; } + /// + /// Reads the per-cell dot 7/8 information liblouis wrote into the scratch typeform buffer. + /// + /// + /// The write-back half of : on a successful forward + /// translation liblouis stores the ASCII character '8' in the slot of every output cell that + /// contains dot 7 or dot 8, and '0' otherwise (lou_translateString.c:1330). Those are + /// characters smuggled through a formtype array, not TypeForm flag values, which is why this + /// converts to booleans instead of exposing the buffer. + /// + private static bool[]? ExtractOutputDots78(TypeForm[]? typeFormBuffer, int outputLength) + { + if (typeFormBuffer is null) + { + return null; + } + + bool[] dots = new bool[outputLength]; + + for (int k = 0; k < outputLength; k++) + { + dots[k] = typeFormBuffer[k] == (TypeForm)'8'; + } + + return dots; + } + /// /// The number of liblouis widechars occupies. /// diff --git a/LibLouis.NET/TranslatedString.cs b/LibLouis.NET/TranslatedString.cs index b043b02..fc4bad0 100644 --- a/LibLouis.NET/TranslatedString.cs +++ b/LibLouis.NET/TranslatedString.cs @@ -3,10 +3,23 @@ public class TranslatedString { public required string Output { get; set; } - + public required int[] OutputPosition { get; set; } - + public required int[] InputPosition { get; set; } - + public required int CursorPosition { get; set; } + + /// + /// Per output cell, whether liblouis reported the cell as containing dot 7 or dot 8. + /// when the translation ran without a formtype array, because liblouis + /// only computes this when one is supplied. + /// + /// + /// This is the write-back half of the native typeform parameter. liblouis writes it per + /// *output* cell, which is why it cannot go into the caller's input-sized formtype array - + /// that write is exactly the buffer overrun the wrapper exists to prevent. Forward + /// translation only: back-translation zero-fills the buffer and reports nothing. + /// + public bool[]? OutputDots78 { get; set; } } From 19113c131e75cb67297bc2c8c969cace2029e250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 11:27:12 +0200 Subject: [PATCH 13/14] test: Hoist the 8-dot table list to a static field Silences CA1861 in the tests added for OutputDots78. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/OutputDotsTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/LibLouis.NET.Test/OutputDotsTests.cs b/LibLouis.NET.Test/OutputDotsTests.cs index 3835a63..290db35 100644 --- a/LibLouis.NET.Test/OutputDotsTests.cs +++ b/LibLouis.NET.Test/OutputDotsTests.cs @@ -15,9 +15,10 @@ namespace LibLouis.NET.Test; /// public class OutputDotsTests { + private static readonly string[] Tables = ["da-dk-braillo.dis", "da-dk-g08.ctb"]; + private static string[] EightDotTables() => - [.. new[] { "da-dk-braillo.dis", "da-dk-g08.ctb" } - .Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))]; + [.. Tables.Select(t => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "tables", t))]; private static TranslatedString TranslateWithTypeForm(string input) { From 9bd00c88388a7544644de787ccb29c8c1d0c9d54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 5 Aug 2026 12:23:57 +0200 Subject: [PATCH 14/14] fix!: Report positions as UTF-16 indices BREAKING: TranslatedString.OutputPosition and InputPosition are now sized to the strings they index - one entry per char of the input and of Output respectively - and are no longer the arrays passed in. Code that slices them by Output.Length still works; code that relied on getting its own array instance back does not. liblouis indexes these arrays in widechars, which on a UCS-4 build - all our binaries - is a whole Unicode character. A .NET string counts UTF-16 code units. The two agree for BMP text and diverge from the first non-BMP character on, and the arrays exist for nothing except indexing strings, so every consumer treated them as UTF-16 indices and was silently misaligned on any input containing an emoji or a musical symbol. An audit of the consuming code found this reaching production: positions are sliced by Output.Length, indexed directly against both strings, and re-based with cross-node offset arithmetic through line-breaking and hyphenation, with no validation that the input is BMP-only. A single emoji corrupts hyphen placement. So translate the values rather than document the hazard. Both halves of a surrogate pair report the same position, values always address the start of a character, and the arrays are sized to their strings so the Output.Length slice that callers write is now exactly the whole array. The cursor is converted in both directions for the same reason. The caller's arrays stay as liblouis wrote them: they are the native scratch buffers, and sizing the results from them is what made the Output.Length slice necessary in the first place. TestPositionResults asserted that the returned array was the same instance as the one passed in. That was describing the implementation, not the contract; it now compares the values over the output's length. Co-Authored-By: Claude Fable 5 --- LibLouis.NET.Test/NativeMethodsTests.cs | 5 +- LibLouis.NET.Test/PositionMappingTests.cs | 181 ++++++++++++++++++++++ LibLouis.NET/LibLouis.cs | 175 +++++++++++++++++++-- LibLouis.NET/TranslatedString.cs | 22 +++ 4 files changed, 372 insertions(+), 11 deletions(-) create mode 100644 LibLouis.NET.Test/PositionMappingTests.cs diff --git a/LibLouis.NET.Test/NativeMethodsTests.cs b/LibLouis.NET.Test/NativeMethodsTests.cs index a203486..b95fa58 100644 --- a/LibLouis.NET.Test/NativeMethodsTests.cs +++ b/LibLouis.NET.Test/NativeMethodsTests.cs @@ -116,7 +116,10 @@ public void TestPositionResults() Assert.Equal(expected, translated.Output); - Assert.Equal(inputPosition, translated.InputPosition); + // The returned arrays are sized to the strings they index rather than to the scratch + // buffers passed in, so InputPosition covers the output and no slicing is needed to use + // it. For this BMP input the values are unchanged from what liblouis wrote. + Assert.Equal(inputPosition[..translated.Output.Length], translated.InputPosition); Assert.Equal(outputPosition, translated.OutputPosition); Assert.Equal('A', input[inputPosition[12]]); diff --git a/LibLouis.NET.Test/PositionMappingTests.cs b/LibLouis.NET.Test/PositionMappingTests.cs new file mode 100644 index 0000000..aae28b8 --- /dev/null +++ b/LibLouis.NET.Test/PositionMappingTests.cs @@ -0,0 +1,181 @@ +using System; +using System.IO; +using System.Linq; + +using Xunit; + +namespace LibLouis.NET.Test; + +/// +/// liblouis indexes its position arrays in widechars - whole Unicode characters on our UCS-4 +/// builds. .NET callers read them as indices into a string, which is UTF-16. The two agree for +/// BMP text and diverge on the first non-BMP character, so the wrapper translates them. +/// +/// +/// The invariant that matters is not "the numbers look right" but that every value is directly +/// usable as a string index: Output[result.InputPosition[k]] must address the character +/// liblouis meant, and must never land on the trailing half of a surrogate pair. +/// +public class PositionMappingTests +{ + 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))]; + + private static TranslatedString Translate(string input) + { + int outputLength = Math.Max(16, input.Length * 4); + + return LibLouis.Instance.Translate( + TablePaths(), + input, + outputLength, + null, + null, + new int[input.Length], + new int[outputLength], + 0, + TranslationMode.Regular); + } + + /// + /// Sized to the strings they index, so no slicing guesswork is needed. + /// + [Theory] + [InlineData("Første linje. Anden linje.")] + [InlineData("bogstaver")] + [InlineData("a\U0001D11Eb")] + [InlineData("\U0001D11E\U0001D11E")] + [InlineData("😀 hej")] + public void ArraysAreSizedToTheStringsTheyIndex(string input) + { + TranslatedString result = Translate(input); + + Assert.Equal(input.Length, result.OutputPosition.Length); + Assert.Equal(result.Output.Length, result.InputPosition.Length); + } + + /// + /// Every InputPosition value must be a usable index into the input string, and must address + /// the start of a character rather than the low half of a surrogate pair. + /// + [Theory] + [InlineData("Første linje. Anden linje.")] + [InlineData("a\U0001D11Eb")] + [InlineData("\U0001D11E\U0001D11E")] + [InlineData("😀 hej")] + public void InputPositionsAddressWholeCharactersOfTheInput(string input) + { + TranslatedString result = Translate(input); + + foreach (int position in result.InputPosition) + { + Assert.InRange(position, 0, input.Length - 1); + Assert.False( + char.IsLowSurrogate(input[position]), + $"position {position} lands on the trailing half of a surrogate pair"); + } + } + + /// + /// The same, in the other direction. + /// + [Theory] + [InlineData("Første linje. Anden linje.")] + [InlineData("a\U0001D11Eb")] + [InlineData("😀 hej")] + public void OutputPositionsAddressWholeCharactersOfTheOutput(string input) + { + TranslatedString result = Translate(input); + + foreach (int position in result.OutputPosition) + { + Assert.InRange(position, 0, result.Output.Length - 1); + Assert.False( + char.IsLowSurrogate(result.Output[position]), + $"position {position} lands on the trailing half of a surrogate pair"); + } + } + + /// + /// Both halves of a surrogate pair belong to the same character, so both must report the same + /// braille cell. + /// + [Fact] + public void SurrogatePairHalvesShareAnOutputPosition() + { + const string input = "a\U0001D11Eb"; + + TranslatedString result = Translate(input); + + // index 1 and 2 are the two halves of U+1D11E + Assert.Equal(result.OutputPosition[1], result.OutputPosition[2]); + + // and the surrounding BMP characters map elsewhere + Assert.NotEqual(result.OutputPosition[0], result.OutputPosition[1]); + } + + /// + /// BMP text must be completely unaffected: widechar and UTF-16 indices coincide there, so the + /// values have to match what liblouis wrote into the caller's scratch array. + /// + [Fact] + public void BmpTextIsUnchanged() + { + const string input = "Første linje. Anden linje, med kursiveret tekst. Tredje linje."; + + int outputLength = input.Length * 4; + + int[] scratchOutput = new int[input.Length]; + int[] scratchInput = new int[outputLength]; + + TranslatedString result = LibLouis.Instance.Translate( + TablePaths(), input, outputLength, null, null, scratchOutput, scratchInput, 0, TranslationMode.Regular); + + Assert.Equal(scratchOutput, result.OutputPosition); + Assert.Equal(scratchInput[..result.Output.Length], result.InputPosition); + } + + /// + /// The pattern that consumer code actually uses, which was correct only for BMP text. + /// + [Theory] + [InlineData("Første linje")] + [InlineData("a\U0001D11Eb")] + public void ConsumerSlicePatternStaysInBounds(string input) + { + TranslatedString result = Translate(input); + + int[] sliced = result.InputPosition[..result.Output.Length]; + + Assert.Equal(result.InputPosition.Length, sliced.Length); + Assert.All(sliced, p => Assert.InRange(p, 0, input.Length - 1)); + } + + /// + /// The cursor comes back as an index into the braille output, so it has to be translated too. + /// + [Fact] + public void CursorPositionIsAnIndexIntoTheOutput() + { + const string input = "a\U0001D11Ebc"; + + int outputLength = input.Length * 4; + + // Cursor on 'b', which sits after the surrogate pair. + TranslatedString result = LibLouis.Instance.Translate( + TablePaths(), + input, + outputLength, + null, + null, + new int[input.Length], + new int[outputLength], + input.IndexOf('b', StringComparison.Ordinal), + TranslationMode.Regular); + + Assert.InRange(result.CursorPosition, 0, result.Output.Length - 1); + Assert.False(char.IsLowSurrogate(result.Output[result.CursorPosition])); + } +} diff --git a/LibLouis.NET/LibLouis.cs b/LibLouis.NET/LibLouis.cs index be4d22b..6d8e661 100644 --- a/LibLouis.NET/LibLouis.cs +++ b/LibLouis.NET/LibLouis.cs @@ -367,13 +367,17 @@ 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. + int[] inputOffsets = Utf16OffsetOfWidechar(input); + int widecharCursor = ToWidecharCursor(input, cursorPosition); + string tables = string.Join(',', tableList); bool success; lock (NativeLock) { ThrowIfShutDown(); - success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; + success = NativeMethods.lou_translate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref widecharCursor, mode) > 0; } if (!success) @@ -381,12 +385,17 @@ public TranslatedString Translate( throw new LibLouisException($"String translation failed: {_lastLogMessage}"); } + string output = ConvertUCSOutputBufferToString(outputBuffer, outputLength); + + (int[] mappedOutputPosition, int[] mappedInputPosition, int mappedCursor) = + MapPositionsToUtf16(input, output, inputOffsets, outputPosition, inputPosition, widecharCursor); + return new TranslatedString { - Output = ConvertUCSOutputBufferToString(outputBuffer, outputLength), - CursorPosition = cursorPosition, - InputPosition = inputPosition, - OutputPosition = outputPosition, + Output = output, + CursorPosition = mappedCursor, + InputPosition = mappedInputPosition, + OutputPosition = mappedOutputPosition, OutputDots78 = ExtractOutputDots78(typeFormBuffer, outputLength), }; } @@ -503,13 +512,17 @@ 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. + int[] inputOffsets = Utf16OffsetOfWidechar(input); + int widecharCursor = ToWidecharCursor(input, cursorPosition); + string tables = string.Join(',', tableList); bool success; lock (NativeLock) { ThrowIfShutDown(); - success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref cursorPosition, mode) > 0; + success = NativeMethods.lou_backTranslate(tables, inputBuffer, ref inputLength, outputBuffer, ref outputLength, typeFormBuffer, spacing, outputPosition, inputPosition, ref widecharCursor, mode) > 0; } if (!success) @@ -517,12 +530,17 @@ public TranslatedString BackTranslate( throw new LibLouisException($"String translation failed: {_lastLogMessage}"); } + string output = ConvertUCSOutputBufferToString(outputBuffer, outputLength); + + (int[] mappedOutputPosition, int[] mappedInputPosition, int mappedCursor) = + MapPositionsToUtf16(input, output, inputOffsets, outputPosition, inputPosition, widecharCursor); + return new TranslatedString { - Output = ConvertUCSOutputBufferToString(outputBuffer, outputLength), - CursorPosition = cursorPosition, - InputPosition = inputPosition, - OutputPosition = outputPosition, + Output = output, + CursorPosition = mappedCursor, + InputPosition = mappedInputPosition, + OutputPosition = mappedOutputPosition, }; } @@ -690,6 +708,143 @@ public string Hyphenate(IEnumerable tableList, string input, Translation return dots; } + /// + /// Converts a cursor given as a .NET string index into the widechar index liblouis expects. + /// + /// + /// Negative means "no cursor" to liblouis and is passed through untouched. + /// + private int ToWidecharCursor(string input, int cursorPosition) + { + if (cursorPosition < 0 || input.Length == 0) + { + return cursorPosition; + } + + int[] widechars = WidecharOfUtf16Offset(input); + + return widechars[Math.Clamp(cursorPosition, 0, input.Length - 1)]; + } + + /// + /// Rewrites liblouis's widechar-indexed position arrays as UTF-16 indices into the managed + /// strings, so every value can be used directly as a string index. + /// + /// + /// liblouis counts in widechars: on a UCS-4 build one widechar is a whole Unicode character, + /// while a .NET string counts UTF-16 code units. The two agree for BMP text and diverge from + /// the first non-BMP character on, which silently misaligns any caller that treats these + /// values as string indices - and the arrays exist for nothing else. + /// + /// The results are sized to the strings they index rather than to the caller's scratch + /// buffers, so OutputPosition has one entry per char of the input and + /// InputPosition one per char of the output. No slicing is required to use them. + /// + /// Both halves of a surrogate pair report the same position, since they are one character. + /// + private (int[] OutputPosition, int[] InputPosition, int CursorPosition) MapPositionsToUtf16( + string input, + string output, + int[] inputOffsets, + int[] outputWidecharPositions, + int[] inputWidecharPositions, + int widecharCursor) + { + int[] outputOffsets = Utf16OffsetOfWidechar(output); + int[] inputWidechars = WidecharOfUtf16Offset(input); + int[] outputWidechars = WidecharOfUtf16Offset(output); + + int lastInputWidechar = Math.Max(inputOffsets.Length - 2, 0); + int lastOutputWidechar = Math.Max(outputOffsets.Length - 2, 0); + + int[] outputPosition = new int[input.Length]; + + for (int i = 0; i < input.Length; i++) + { + int widechar = inputWidechars[i]; + + int cell = widechar < outputWidecharPositions.Length ? outputWidecharPositions[widechar] : 0; + + outputPosition[i] = outputOffsets[Math.Clamp(cell, 0, lastOutputWidechar)]; + } + + int[] inputPosition = new int[output.Length]; + + for (int t = 0; t < output.Length; t++) + { + int widechar = outputWidechars[t]; + + int character = widechar < inputWidecharPositions.Length ? inputWidecharPositions[widechar] : 0; + + inputPosition[t] = inputOffsets[Math.Clamp(character, 0, lastInputWidechar)]; + } + + // A negative cursor means "no cursor" to liblouis; leave it alone. + int cursorPosition = widecharCursor < 0 || output.Length == 0 + ? widecharCursor + : outputOffsets[Math.Clamp(widecharCursor, 0, lastOutputWidechar)]; + + return (outputPosition, inputPosition, cursorPosition); + } + + /// + /// The UTF-16 offset at which each widechar of starts, with a + /// sentinel holding the string's length at the end. + /// + private int[] Utf16OffsetOfWidechar(string value) + { + int[] offsets = new int[CountUCSCharacters(value) + 1]; + + int widechar = 0; + + for (int i = 0; i < value.Length; widechar++) + { + offsets[widechar] = i; + i += IsSurrogatePairAt(value, i) ? 2 : 1; + } + + offsets[widechar] = value.Length; + + return offsets; + } + + /// + /// The widechar that each UTF-16 offset of belongs to. Both halves of + /// a surrogate pair map to the same widechar, because they are one character to liblouis. + /// + private int[] WidecharOfUtf16Offset(string value) + { + int[] widechars = new int[value.Length]; + + int widechar = 0; + + for (int i = 0; i < value.Length; widechar++) + { + int width = IsSurrogatePairAt(value, i) ? 2 : 1; + + for (int k = 0; k < width; k++) + { + widechars[i + k] = widechar; + } + + i += width; + } + + return widechars; + } + + /// + /// Whether a surrogate pair - one widechar, two chars - starts at . + /// Never true on a UCS-2 build, where a widechar is a UTF-16 code unit. + /// + private bool IsSurrogatePairAt(string value, int index) + { + return CharacterSize == 4 + && char.IsHighSurrogate(value[index]) + && index + 1 < value.Length + && char.IsLowSurrogate(value[index + 1]); + } + /// /// The number of liblouis widechars occupies. /// diff --git a/LibLouis.NET/TranslatedString.cs b/LibLouis.NET/TranslatedString.cs index fc4bad0..4e82eab 100644 --- a/LibLouis.NET/TranslatedString.cs +++ b/LibLouis.NET/TranslatedString.cs @@ -4,10 +4,32 @@ public class TranslatedString { public required string Output { get; set; } + /// + /// For each char of the input, the index into it translated to. One entry + /// per char, so OutputPosition.Length equals the input's length and no slicing is + /// needed. Both halves of a surrogate pair report the same position. + /// + /// + /// A UTF-16 index, usable directly against the strings. liblouis reports these in widechars - + /// whole characters on a UCS-4 build - which agrees with UTF-16 only for BMP text; the + /// wrapper translates them. This is not the array passed in, which stays as liblouis wrote it. + /// public required int[] OutputPosition { get; set; } + /// + /// For each char of , the index into the input it came from. One entry per + /// char, so InputPosition.Length equals Output.Length. + /// + /// + /// A UTF-16 index, on the same terms as . Values always address + /// the start of a character, never the trailing half of a surrogate pair. + /// public required int[] InputPosition { get; set; } + /// + /// Where the cursor ended up, as an index into . Negative when the + /// translation was given no cursor. + /// public required int CursorPosition { get; set; } ///