Skip to content

Fix undefined behavior in RLE encode/decode size header handling - #399

Merged
Screwtapello merged 1 commit into
bsnes-emu:masterfrom
vimac:fix-rle-size-header-ub
Sep 27, 2026
Merged

Screwtapello merged 1 commit into
bsnes-emu:masterfrom
vimac:fix-rle-size-header-ub

Conversation

@vimac

@vimac vimac commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

load() returns uint8_t, which promotes to int; load() << byte * 8 performs a shift of >= 32 bits on a 32-bit int when byte >= 4, which is undefined behavior. On the encode side, input.size() >> byte * 8 has the same problem, since input.size() is a 32-bit uint.

At -O2/-O3 (the performance build profile), Apple Clang miscompiles nall::Decode::RLE based on this UB and emits a brk trap at runtime: saving states appears to work, but loading a state — or even just selecting one in the State Manager, which RLE-decodes the preview image — crashes bsnes. -O1/-Og builds happen to generate working code, which made this look like a macOS-specific regression (see #396 #296).

This was reproduced independently of the UI: decoding a valid .bst state file with the exact updateSelection() preview code crashes at -O2/-O3 and works at -O1/-Os; casting to uint64_t before shifting fixes all optimization levels.

Cast to uint64_t before shifting so all eight size-header bytes are assembled and written correctly at any optimization level.

Fixes #396 #296

load() returns uint8_t, which promotes to int; 'load() << byte * 8'
performs a shift of >= 32 bits on a 32-bit int when byte >= 4, which
is undefined behavior. On the encode side, 'input.size() >> byte * 8'
has the same problem, since input.size() is a 32-bit uint.

At -O2/-O3 (the performance build profile), Apple Clang miscompiles
nall::Decode::RLE based on this UB and emits a brk trap at runtime:
saving states appears to work, but loading a state — or even just
selecting one in the State Manager, which RLE-decodes the preview
image — crashes bsnes. -O1/-Og builds happen to generate working
code, which made this look like a macOS-specific regression.

Cast to uint64_t before shifting so all eight size-header bytes are
assembled and written correctly at any optimization level.

Fixes bsnes-emu#396.
@Screwtapello

Copy link
Copy Markdown
Contributor

Thank you very much for looking into this! I'll try to review and merge this soon.

@Screwtapello

Copy link
Copy Markdown
Contributor

Building latest version on Linux with g++ 16: seems to work fine, as expected
Building latest version with clang++ 21: crash when loading save state
Building this patch with g++ 16: still works fine
Building this patch with clang++ 21: now works!

Thank you for your contribution!

@Screwtapello
Screwtapello merged commit 0c2fa0d into bsnes-emu:master Sep 27, 2026
4 of 5 checks passed
@vimac
vimac deleted the fix-rle-size-header-ub branch September 27, 2026 08:35
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.

Save states broken on MacOS

2 participants