Skip to content

arm64 and Android guests: string literals kept byte for byte (fixes a voice chat crash) - #346

Merged
bnunu merged 2 commits into
OpenCommunityEdition:mainfrom
startupfoundry:arm64-guest-strings
Oct 16, 2026
Merged

bnunu merged 2 commits into
OpenCommunityEdition:mainfrom
startupfoundry:arm64-guest-strings

Conversation

@startupfoundry

@startupfoundry startupfoundry commented Oct 14, 2026 •

Copy link
Copy Markdown

The arm64 Linux build (Steam Frame) crashed in an internet CTF game as soon as another player's voice arrived. The cause wasn't Opus or voice chat: the guest's assembly converter was dropping _ bytes from string literals, which shifted data tables in every arm64 and Android guest build.

The crash

A client on a Steam Frame crashed in decode_pulses with signal 11 at address 0x3c3. The call chain was network_voice_handle_message → voice_audio_play → opus_decode_float, decoding a teammate's packet. decode_pulses was called with n=32, k=240. CELT_PVQ_U_ROW has 15 rows, so the lookup read a garbage pointer.

libopus is fuzzed and never reaches those values from any packet. The data it uses to bound pulses was wrong.

Why

The arm64 Linux and Android guests are compiled for arm64_32-apple-watchos, and tools/android_asm_convert.py rewrites the Darwin assembly as ELF. Two bugs in the converter combined:

  • String operands were treated as symbols. Every operand of .ascii/.asciz went through the symbol renamer. That took a quoted string for a quoted symbol name and stripped its leading _, as Darwin symbols carry one.
  • The quoted-name pattern ignored escapes. "[^"]*" ended at an escaped \", so the _ of any word after a 0x22 byte was stripped too.

Clang emits byte tables as .ascii. Any table holding 95 (_) lost that byte, and everything after it moved down by one.

In Opus that table is CELT's pulse cache (celt/modes.c, cache_bits50). Shifted by a byte, it lets a band take more pulses than the PVQ tables hold. The guest's encoder can crash the same way in encode_pulses. Before this fix, a Frame player's voice probably reached others as noise.

The same loss hit other guest data. No visible failures were confirmed, but all of it is now assembled as compiled:

  • musl's iswalpha/iswpunct/towctrans tables and iconv charmaps;
  • tomlc17's "_0123456789…" digit sets (TOML numbers with _);
  • "_de"/"_fr" (attract mode), "_lan_", "_lit", "_%s";
  • the progress bar's compressed monk tone and the DirectSound effects image;
  • assertion and error strings.

embed_assets.py already avoided .ascii because "the converter would garble" it, which was this bug.

What changes (2 commits)

  1. The converter keeps string literals byte for byte.
    • .ascii/.asciz/.string operands pass through unchanged.
    • Quoted names honour escapes.
    • New tools/test_android_asm_convert.py is added to CI's pytest line. It has unit cases, plus an end-to-end table of all 256 byte values and a _-led string through clang (arm64_32) → the converter → the assembler, compared in .rodata.
    • The Android README's converter step and the stale embed_assets.py comment are updated.
  2. Voice chat drops a packet that isn't one 20 ms frame before decoding (voice_audio_play, using opus_packet_get_nb_samples).
    • Every build sends exactly one 20 ms frame per packet, so the wire format is unchanged.
    • A dropped packet is concealed like a lost one.
    • This is defence in depth: the crash came from valid packets and the garbled table.

Tested

  • Reproduced off the device. A harness links the game's own Opus objects, built by the guest pipeline: same flags, clang → converter → as. It maps the ILP32 image below 4 GB, as the host does, and drives it with the game's settings: VOIP, 960-sample frames, in-band FEC, 10% loss, complexity 5, 8–64 kbps. It feeds clean, bit-flipped, truncated, extended and random packets, through voice_audio_play's sequence, concealment and FEC logic.
    • Before: it crashed after 78 decodes, at 0x3c3 with n=32, k=240, matching the device. Clean encoder packets alone crashed in encode_pulses after ~2,500 frames. At -O0 it crashed identically, which rules out the optimizer.
    • After: 4,800,436 decodes over 5 seeds × 250,000 frames, with no fault.
    • The same harness built natively (LP64, ASan+UBSan) ran 3.07 M decodes clean before the fix, which rules out Opus 1.5.2 itself and the call pattern.
  • Wire-identical. After the fix, the guest's encoded bytes and decoded samples are bit-identical to a native build's (FNV hashes, -ffp-contract=off).
  • The converter's output had 60 string directives that differed from clang's before; now it has 0.
  • tools/test_android_asm_convert.py: 7 passed with the fix. With the old converter, 4 fail.
  • Builds and VR checks: the arm64 Linux VR build (configure.py --vr) and the flat build compile. The VR build's holsters, menu, reload, crouch, death and cinematic checks pass, and it's running on the Steam Frame.

Not tested yet

  • The Android build on a device. It uses the same converter, so it should benefit the same way.
  • This workflow on GitHub Actions.

The arm64 and Android guests are compiled for arm64_32-apple-watchos and
their Darwin assembly converted to ELF (tools/android_asm_convert.py). The
converter took a quoted .ascii/.asciz operand for a quoted symbol name and
dropped a leading "_" from it, as it does from symbols; and its quoted-name
pattern ended at an escaped quote (\"), so the "_" of a word after one was
dropped too. Clang writes byte tables as .ascii strings, so a table holding
95 ("_") lost that byte and everything after it moved down by one.

In Opus that table was CELT's pulse cache (modes.c, cache_bits50): the bits
a band may have are read off it, and shifted by a byte it lets a band have
more pulses than the PVQ tables hold. Decoding a frame from another player
then read past CELT_PVQ_U_ROW in decode_pulses (n 32, k 240) and crashed: a
client on the Steam Frame, in a CTF game on the internet. The guest's own
encoder could crash likewise in encode_pulses. The same build of Opus,
driven with the game's settings and with clean, corrupt, truncated and
random packets and voice_audio_play's loss handling, crashed within a few
thousand frames (and identically at -O0); with the fix it ran 4.8 million
decodes clean, and its encoded bytes and decoded samples are identical to a
native 64-bit build's.

The same loss hit other strings and tables of the guest: musl's iswalpha,
iswpunct and towctrans tables and iconv's charmaps; tomlc17's "_0123..."
digit sets (TOML numbers with "_"); the attract mode's "_de"/"_fr" names,
"_lan_", "_lit" and "_%s"; the progress bar's compressed monk tone and the
DirectSound effects image; and assertion and error texts. All are now
assembled as compiled.

String directives now go through unchanged, and quoted names honour
escapes. tools/test_android_asm_convert.py checks both, and a table of
every byte value through clang, the conversion and the assembler.
…coding

Every build sends one 20 ms frame per packet (voice_audio_encode). A packet
whose table of contents says otherwise, or that Opus cannot parse, is
damaged: voice_audio_play now drops it before the decoder sees it
(opus_packet_get_nb_samples), and the next frame makes up for it as lost.
Cheap defence in depth: Opus's decoder already refuses such packets, and the
crash in decode_pulses came from a valid packet and the guest's garbled
pulse cache, fixed in the assembly conversion.
@bmills23

Copy link
Copy Markdown

This happens to me every time I matchmake. Bumping this PR for visibility cuz I don't want MTV Cribs to crash my game every 5 seconds.

@meowsandstuff
meowsandstuff self-requested a review October 16, 2026 03:12
@meowsandstuff

Copy link
Copy Markdown
Collaborator

I'm in favour of this. It's a well-scoped fix. The only thing I want to see is that this is actually tested prior to merging, preferably built and play-tested on an Android device to ensure that no further crashes occur.

@meowsandstuff meowsandstuff added help wanted Extra attention is needed priority: medium Normal priority labels Oct 16, 2026
@bnunu
bnunu self-requested a review October 16, 2026 03:20

@bnunu bnunu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will merge upon successful testing

@bnunu
bnunu merged commit 085676d into OpenCommunityEdition:main Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed priority: medium Normal priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants