Separate Nota's tables from the upstream set - #10
Open
henrikottesorensen wants to merge 15 commits into
Open
Conversation
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Silences CA1861 in the tests added for OutputDots78. Co-Authored-By: Claude Fable 5 <[email protected]>
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 <[email protected]>
LibLouis.NET.Test/tables held thirty of Nota's Danish tables while LibLouis.NET.Tables copies the upstream set into the same output directory. Twenty two of the thirty share a file name with an upstream table and differ from it, so which copy a test got depended on MSBuild item ordering, and nothing said so anywhere. Moves them to nota-tables/, kept separate in the output, and points every test at that path. Each test now states which set it means, and the upstream tables are no longer shadowed, which is what lets upstream's braille specs be checked against them. Nota's tables include seven general upstream tables, and liblouis resolves an include relative to the directory of the table doing the including, so those are copied in alongside. They come from the staged upstream set rather than being committed, so they cannot drift from it. NotaTablesAreSelfContained asserts that list stays complete, since an explicit list goes stale the moment a table gains an include. The second guard checks that none of Nota's tables reach tables/. Sharing a file name across the two directories is expected - most of Nota's are forks of an upstream table of the same name - so the tables that exist nowhere upstream are the canary: if one appears in tables/, the two sets are being copied to the same place again. No table content changes. Twenty three of the thirty are reachable from no test - they are a stale copy of the set the application ships - but they are kept and documented rather than deleted, because deciding what is canonical is not this change's business. Co-Authored-By: Claude Opus 5 <[email protected]>
henrikottesorensen
force-pushed
the
chore/drop-stale-test-tables
branch
from
August 5, 2026 10:31
bd87f25 to
7420946
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased onto #9
The first 14 commits are that PR. Review only the last one,
test: Separate Nota's tables from the upstream set.Merge #9 first and this reduces to a single commit.
LibLouis.NET.Test/tables/held thirty of Nota's Danish tables, whileLibLouis.NET.Tablescopies the upstream set into the same output directory. 22 of the 30 share a file name with an upstream table and differ from it, so which copy a test got depended on MSBuild item ordering — and nothing said so anywhere.This moves them to
nota-tables/, kept separate in the output, and points every test at that path.Why it matters beyond tidiness
The shadowing has already cost real time. Running upstream's Danish braille specs against the wrapper failed 2,699 forward cases until they were redirected at an unshadowed upstream directory — the tables under test were Nota's, not the ones the specs were written for. With this change
tables/is pure upstream and can be checked against directly.It also surfaced something worth knowing: Nota's tables are a fork of an older upstream, not a patch on the current one. They add what upstream lacks — the
foreignemphasis class behindTypeForm.ForeignLanguage— and are missing what upstream has since fixed, such as the rules removing the space between§and a following number.da-dk-g2.dicdiverges by 14,810 lines. Two files exist nowhere upstream, which is why the tests cannot simply use the upstream set.Rebased onto #9, and why that way round
#9 adds ten test files that build paths from
"tables", the directory this renames. Rebasing this onto #9 rather than the reverse means #9 needs no churn: this PR absorbs the mechanical path update across those ten files.That also unblocked something. The first version of this PR deliberately shipped without a self-containment test, because adding a second test class to the assembly crashed the run on
main:That is #9's log-callback fix, item 5 of the audit — one more allocating test class was enough to trigger a GC and collect the unrooted delegate. On top of #9 it is gone, so both guards are included here.
63 tests pass on the combination.
Guards
NotaTablesAreSelfContained— liblouis resolves anincluderelative to the directory of the table doing the including, so Nota's tables need the seven general upstream tables they include sitting alongside them. Those are copied from the staged upstream set rather than committed, so they cannot drift. The list is explicit in the csproj, and an explicit list goes stale the moment a table gains an include, so this asserts the closure holds.NotaTablesDoNotLeakIntoTheUpstreamDirectory— sharing a file name across the two directories is expected. What must not recur is both sets landing in one directory, so the tables that exist nowhere upstream act as the canary.Nothing deleted
23 of the 30 are reachable from no test — a stale copy of the set the application ships, drifting independently of both upstream and production. They are kept and documented in
nota-tables/README.mdrather than deleted; deciding what is canonical isn't this change's business.🤖 Generated with Claude Code