Skip to content

Marshal spacing as the output buffer it is - #19

Merged
henrikottesorensen merged 1 commit into
mainfrom
fix/spacing-output-buffer
Oct 9, 2026
Merged

henrikottesorensen merged 1 commit into
mainfrom
fix/spacing-output-buffer

Conversation

@henrikottesorensen

Copy link
Copy Markdown
Collaborator

Rebased onto #9

The first 15 commits are that PR. Review only the last one, fix: Marshal spacing as the output buffer it is.

Merge #9 first and this reduces to a single commit.

Backlog item 7 from the audit notes: "Fix spacing, or hard-code null and document why." This fixes it, as an output buffer.

What was wrong

liblouis's spacing is in/out — it answers in the same buffer it reads the request from. The wrapper declared it as string? with UTF-8 marshalling, which produced two separate defects:

  1. The answer was unreachable. liblouis dutifully computed spacing information and wrote it into the marshaller's temporary buffer, which is freed after the call. A .NET string is immutable, so nothing could ever be copied back. The parameter looked like it worked — no error, translation succeeded — but no caller could ever receive spacing information. It was a feature that could not function.

  2. The buffer was undersized, in the direction nobody looked at. The audit notes recorded this as a one-byte overrun on the forward path. Reading the implementation, the back-translation path is much worse.

The header is wrong about the buffer size

liblouis.h.in promises spacing need only be "NULL or at least inlen elements long". The two directions disagree, and neither matches that:

Direction What it writes Needs
Forward memcpy(srcSpacing, destSpacing, input.length) then srcSpacing[input.length] = 0 (lou_translateString.c:1384-1385) inlen + 1
Backward memset(spacebuf, '*', *outlen) up front, then writes per output cell (lou_backTranslateString.c:229, 1073) outlen

So back-translation overran by outlen - inlen bytes. For a 27-character braille input with an outlen of 108 — an ordinary call, and exactly what the round-trip in the new tests does — that is a 80-byte native heap overrun, not one byte.

Buffers are now sized max(inputLength, outputLength) + 1, which is the same shape PrepareTypeFormBuffer already uses for the identical problem in #9.

Two more details the header does not mention: it is a char buffer rather than widechar, and liblouis indexes it in widechars, so a per-char request has to collapse surrogate pairs on the way in. Counting in UTF-16 units would describe a longer buffer than the one allocated — the same trap as the widechar length fixes in #9.

How the answer is reported

TranslatedString.OutputSpacing, following OutputDots78 from #9. That is not an arbitrary choice: the typeform write-back has the same shape — in/out, indexed per output cell — and the same reason it cannot be written back into the caller's input-sized argument. Reusing the pattern keeps all four public signatures unchanged, so this is not a breaking change; string? spacing stays exactly as it was.

The two overloads that return a bare string have nowhere to report it, so they compute and discard it, as they already do for OutputDots78. Their buffers still have to be sized correctly, and are.

Values are one char per output cell: '*' where liblouis reported nothing, an ASCII digit carried over from the input character that produced the cell, or '1' where back-translation inserted a space. Verified:

input    (34): Anden linje, med kursiveret tekst.
request  (34): 0000003330000000000000000000000000
output   (27): @anç linje, m kursi#rò ükz.
spacing  (27): **0003330000000000000000000

The three '3's land on output cells 5–7 — exactly the cells produced by the three input characters that were marked.

One limitation kept rather than papered over

Forward translation copies its answer back over only the first inlen bytes, so when a translation grows the text the cells past the input's length hold no answer, and OutputSpacing is shorter than Output. Back-translation reports the full output. That asymmetry is upstream's, not the wrapper's; it is documented on the property and pinned by a test in each direction rather than hidden behind padding.

A leading 'X' in the request tells liblouis to skip the computation entirely (lou_translateString.c:1203). That now surfaces honestly as OutputSpacing == null instead of echoing the caller's own request back.

Tests

Nine cases in LibLouis.NET.Test/SpacingTests.cs:

  • per-output-cell reporting, cross-checked against InputPosition
  • null when not requested
  • the 'X' disable sentinel
  • the back-translation round trip that was the 80-byte overrun
  • surrogate-pair collapsing (one entry per character, not per UTF-16 unit)
  • the request-length guard
  • both non-position overloads, forward and backward

70 tests pass.

Unchanged from 3.33.0 to 3.38.0 — the 3.38 source is identical here apart from an added pos < input->length bounds check, so #11 does not affect this.

Worth reporting upstream

Two things, alongside the table-compilation stack depth already queued: the header's documented spacing size is wrong for back-translation in a way that overruns a conforming caller's buffer, and memcpy(srcSpacing, destSpacing, input.length) reads out of bounds of liblouis's own destSpacing when inlen exceeds the allocation sized from outlen.

🤖 Generated with Claude Code


Recreated from #17 to move the head branch onto Notalib/LibLouis.NET, which GitHub stacked PRs require (stacks cannot span forks).

Base automatically changed from fix/pinvoke-audit to main October 9, 2026 13:19
@nanchen2483
nanchen2483 force-pushed the fix/spacing-output-buffer branch from dd32cb0 to 70c9069 Compare October 9, 2026 13:19
liblouis's spacing parameter is in/out: it answers in the same buffer it
reads the request from. It was declared as a string, so the answer landed
in the marshaller's temporary and was freed unread - no caller could ever
receive spacing information - and the buffer was sized to the input while
liblouis writes per output cell.

Sizing is max(inputLength, outputLength) + 1, not the "at least inlen
elements" the header promises, because the two directions disagree.
Forward writes inlen + 1 bytes (lou_translateString.c:1385);
back-translation opens with memset(spacing, '*', *outlen)
(lou_backTranslateString.c:229) and then writes per output cell. An
inlen-sized buffer overran by outlen - inlen bytes backwards - 80 bytes
for a 27-char input with an outlen of 108, not the one byte the header
implies.

It is also a char buffer rather than widechar, and liblouis indexes it in
widechars, so the caller's per-char request collapses to one entry per
character on the way in.

The answer surfaces as TranslatedString.OutputSpacing, following
OutputDots78: the typeform write-back has the same shape, and the same
reason it cannot go back into the caller's own argument. All four public
signatures are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@henrikottesorensen
henrikottesorensen force-pushed the fix/spacing-output-buffer branch from 70c9069 to 7c6f1b8 Compare October 9, 2026 13:45
@henrikottesorensen
henrikottesorensen merged commit 6a4738f into main Oct 9, 2026
10 checks passed
henrikottesorensen added a commit to henrikottesorensen/LibLouis.NET that referenced this pull request Oct 9, 2026
liblouis 3.39.0 deprecates the spacing parameter of lou_translate,
lou_translateString, lou_backTranslate and lou_backTranslateString: it
never worked reliably and has no known users. Upstream keeps it for API
and ABI compatibility, so the P/Invoke declarations are unchanged.

C# cannot obsolete a single parameter, so each of the four public
methods gains an overload without spacing, and the existing overloads
are marked [Obsolete]. Those still pass spacing through the buffer
handling from Notalib#19, so existing callers behave exactly as before and
only see a CS0618 warning. Both forward to a private core method that
holds the shared body.

TranslatedString.OutputSpacing is left unmarked: it is only non-null
when a caller used an obsolete overload, which already warned, and
marking it would force a suppression around the library's own
initializers. Its documentation points at the deprecation instead.

The tests move to the new overloads. SpacingTests exercises the
obsolete overloads on purpose, so it suppresses CS0618.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
henrikottesorensen added a commit to henrikottesorensen/LibLouis.NET that referenced this pull request Oct 9, 2026
liblouis 3.39.0 deprecated the spacing parameter of lou_translate,
lou_translateString, lou_backTranslate and lou_backTranslateString, and
in practice ignores it: each function now only logs "the spacing
parameter is deprecated and ignored; pass NULL" and never touches the
buffer. The release notes say deprecated, the source says ignored.

That broke Notalib#19 on 3.39.0. OutputSpacing was read back from a buffer
liblouis no longer writes, so it reported the caller's own request as
if it were liblouis's answer, and every call with a spacing argument
logged the warning. Two SpacingTests failed on main as a result.

C# cannot obsolete a single parameter, so each of the four public
methods gains an overload without spacing, which now holds the body,
and the existing overloads are marked [Obsolete] and forward to it,
dropping spacing. liblouis is always handed NULL, so nothing is logged.
The obsolete overloads no longer check the length of spacing either,
since there is nothing left for a mismatch to break.

The spacing buffer helpers and TranslatedString.OutputSpacing from Notalib#19
are removed. OutputSpacing never shipped in a release, and it could
only ever be null now.

SpacingTests is rewritten for the new contract: the obsolete overloads
give the same results as the new ones, and liblouis logs no spacing
warning. The rest of the tests move to the new overloads.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants