Marshal spacing as the output buffer it is - #17
Closed
henrikottesorensen wants to merge 1 commit into
Closed
Conversation
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>
Collaborator
Author
|
Superseded by #19. The head branch has moved from the |
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
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 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-codenulland document why." This fixes it, as an output buffer.What was wrong
liblouis's
spacingis in/out — it answers in the same buffer it reads the request from. The wrapper declared it asstring?with UTF-8 marshalling, which produced two separate defects: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.
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.inpromisesspacingneed only be "NULL or at leastinlenelements long". The two directions disagree, and neither matches that:memcpy(srcSpacing, destSpacing, input.length)thensrcSpacing[input.length] = 0(lou_translateString.c:1384-1385)inlen + 1memset(spacebuf, '*', *outlen)up front, then writes per output cell (lou_backTranslateString.c:229, 1073)outlenSo back-translation overran by
outlen - inlenbytes. For a 27-character braille input with anoutlenof 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 shapePrepareTypeFormBufferalready uses for the identical problem in #9.Two more details the header does not mention: it is a
charbuffer 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, followingOutputDots78from #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? spacingstays exactly as it was.The two overloads that return a bare
stringhave nowhere to report it, so they compute and discard it, as they already do forOutputDots78. 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: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
inlenbytes, so when a translation grows the text the cells past the input's length hold no answer, andOutputSpacingis shorter thanOutput. 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 asOutputSpacing == nullinstead of echoing the caller's own request back.Tests
Nine cases in
LibLouis.NET.Test/SpacingTests.cs:InputPositionnullwhen not requested'X'disable sentinel70 tests pass.
Unchanged from 3.33.0 to 3.38.0 — the 3.38 source is identical here apart from an added
pos < input->lengthbounds check, so #11 does not affect this.Worth reporting upstream
Two things, alongside the table-compilation stack depth already queued: the header's documented
spacingsize is wrong for back-translation in a way that overruns a conforming caller's buffer, andmemcpy(srcSpacing, destSpacing, input.length)reads out of bounds of liblouis's owndestSpacingwheninlenexceeds the allocation sized fromoutlen.🤖 Generated with Claude Code