fix(osc): decode OSC payloads to UTF-16 instead of aborting on bad bytes - #267
fix(osc): decode OSC payloads to UTF-16 instead of aborting on bad bytes#267GlassOnTin wants to merge 3 commits into
Conversation
invokeOscSequence hands the raw OSC payload to NewStringUTF. That call does not fail politely: ART treats invalid modified UTF-8 as fatal and aborts the process. There is no exception to catch — the app is simply gone. An OSC payload is whatever the remote program emitted. A window title from a non-UTF-8 Windows console has no obligation to be valid UTF-8, so this is reachable from any hostile or merely mis-encoded remote. Decoded to UTF-16 and passed to NewString, which has no such failure mode. Malformed sequences become U+FFFD: a terminal showing a replacement character for a mis-encoded title is behaving correctly; one that kills the app is not. Routing through the existing utf8_to_mutf8 would not have worked. It assumes well-formed input — a lone continuation byte is copied through unchanged, so NewStringUTF would still abort — and its 4-byte branch reads p[1..3] without checking them against the end of the buffer, so a truncated sequence at the end of the input reads past it. The new decoder length-checks every sequence before touching a continuation byte, and rejects overlong encodings and encoded surrogates. Also sized from the string rather than c_str(), so a payload containing an embedded NUL is passed through whole instead of silently truncated. What the tests do and do not prove. They run on the host JVM, whose NewStringUTF is LENIENT — it returns mojibake rather than aborting. Only ART aborts. Measured: with this fix reverted, the three liveness tests still pass, so they cannot catch the abort and are not evidence against it. malformedBytesBecomeReplacementCharacters is the one that can fail, and does. It drives an OSC 1337 annotation whose payload is readable back as segment metadata, and asserts the bad byte arrives as U+FFFD with the valid text either side intact. Confirmed failing against the unfixed tree and passing with the fix. Full suite 374 tests, 0 failures.
Diagnostics, mostly — this release exists so the next crash report can answer its own question. #509 and #517 are both native crashes and both stalled on the same missing evidence: the reporter's log ends at `Fatal signal ...` with no backtrace. That was Haven's fault. It captured logcat from inside its own process, so a native signal killed the recorder too and the tombstone landed after Haven was gone. ApplicationExitInfo now recovers it on the next launch, surfaced in Settings -> Connection log with a copy button and over MCP as get_native_crashes. Neither reporter should have to wire up adb to tell me where their app died. Also carried: OSC payloads are decoded to UTF-16 rather than handed to NewStringUTF, which aborts the process on anything that isn't valid modified UTF-8. A window title from a non-UTF-8 Windows console does that as a matter of course, so this was reachable from any mis-encoded remote. Upstreamed as connectbot/termlib#267, along with an out-of-bounds read found next to it in utf8_to_mutf8 that is flagged there rather than silently bundled. Not device-verified: producing a real tombstone means crashing a real build on a real phone. The bookkeeping (dedup, cap, ordering) is unit-tested and each failure mode was reintroduced and confirmed to fail a named test; the framework call itself is not covered.
CI runs spotlessKotlinCheck, which Haven's fork does not, so this only showed up here. Single-expression function on one line.
The payload passed to invokeOscSequence is arbitrary data from the remote end, and it was written to logcat in full at debug level. OSC 52 is the clipboard: termSelectionSet passes the *decoded* copied text through this same call. So anything copied in a remote session — a password out of a manager, an API token, a private key — was recorded in plaintext. OSC 3008 carries user, hostname and cwd for the same ride. Another app cannot read the log on Android 4.1+, so this is not silent harvesting; the exposure is sharing. `adb logcat` and `adb bugreport` capture it, and any app that surfaces its own logs for diagnostics — or asks users to attach one to a bug report — carries it straight out. Command number and payload length are kept. They are what make the line useful for sequencing and truncation bugs, and neither reveals content. Reported downstream by a user who had to hand-redact their own log before it was safe to attach to an issue.
|
Added a second commit to this PR: It's the same function and the same argument about untrusted remote data, so it seemed to belong here rather than in a separate PR — happy to split it out if you'd rather review them independently. The reason it matters more than a typical log tidy-up: OSC 52 is the clipboard. Not silent harvesting: another app can't read the log on Android 4.1+. The exposure is sharing — Command number and length are kept, since that's what makes the line useful for sequencing and truncation bugs and neither reveals content. Both commits verified together: 374 tests, 0 failures, and |
invokeOscSequencehands the raw OSC payload toNewStringUTF. That call doesn't fail politely — ART treats invalid modified UTF-8 as fatal and aborts the process. There's no exception to catch and nothing the Kotlin side can do about it; the app is just gone.An OSC payload is whatever the remote program chose to emit. A window title from a non-UTF-8 Windows console has no obligation to be valid UTF-8, so this is reachable from any hostile — or merely mis-encoded — remote host.
This decodes to UTF-16 and uses
NewString, which has no such failure mode. Malformed sequences become U+FFFD. A terminal that shows a replacement character for a mis-encoded title is behaving correctly; one that kills the app is not.Why not route through
utf8_to_mutf8That was my first instinct, and it doesn't work — for two reasons that are worth flagging on their own:
It assumes well-formed input. A lone continuation byte falls through to the "1, 2, or 3 byte sequence" branch and is copied verbatim, so the output is still invalid and
NewStringUTFwould still abort.Its 4-byte branch reads out of bounds.
(c & 0xF8) == 0xF0leads to readingp[1],p[2],p[3]with no check that those bytes are within the buffer. A truncated 4-byte sequence at the end of the input reads past the end. Pass 1 has the same shape (p += 4unchecked).I have not touched
utf8_to_mutf8in this PR — the new decoder doesn't use it, and I didn't want to bundle an unrelated change into a bug fix. But (2) is a live out-of-bounds read in the tree regardless of this PR, and I'm happy to send a separate one for it if you'd like. It's reachable fromsetTermPropwith a string property whose last character is a truncated 4-byte sequence.The new decoder length-checks every sequence before touching a continuation byte, and rejects overlong encodings and encoded surrogates.
Also sized from the
std::stringrather thanc_str(), so a payload containing an embedded NUL is passed through whole instead of silently truncated at the NUL.What the tests prove, and what they don't
Worth being explicit, because the green tick is misleading on its own:
The tests run on the host JVM, whose
NewStringUTFis lenient — it returns mojibake rather than aborting. Only ART aborts. I measured this: with the fix reverted, the three "does it survive" tests still pass. They cannot catch the abort and are not evidence against it. I've said so in the test file rather than leave a passing test implying coverage that isn't there.malformedBytesBecomeReplacementCharactersis the one that genuinely fails without the fix. It drives an OSC 1337 annotation — which reaches this exact code path and stores its payload as readable segment metadata — and asserts the bad byte arrives as U+FFFD with the valid text either side intact. Confirmed failing against the unfixed tree, passing with the fix.Full suite: 374 tests, 0 failures.
Found while investigating a native SIGABRT report downstream. It is explicitly not that crash — in the reported trace every OSC call completed, and the log line is emitted before the string is created, so an abort here would have left a call with no matching return. Sending this on its own merits.