OOB heap write in RLE/player/shadow bitmap decoders on malformed input

[jussiohag]

Summary

The RLE, player, and shadow bitmap decoders read a run length from the file and write that many pixels without bounding the write index against the row width. setPixel / PixelBuffer::set have no bounds check (only an assert in calcIdx, compiled out in release builds), so a malformed LST/BOB bitmap with a run count that pushes x past the row produces an out-of-bounds heap write into the pixel buffer. This is a robustness/crash-safety issue when loading untrusted or corrupt asset files.

Three sibling decoders share the same defect:

File	Line	Decoder
src/ArchivItem_Bitmap_RLE.cpp	~85–89	RLE — inner for(i<count; ++x) setPixel(x,y,...), x never compared to width; first count byte also read before the size guard
src/ArchivItem_Bitmap_Player.cpp	~137–171	Player — x unbounded vs width; position only checked once per row (~L133), not per read
src/ArchivItem_Bitmap_Shadow.cpp	~88–89	Shadow — for(i<count; ++x) buffer.set(x,y,gray), x unbounded vs width

For comparison, ArchivItem_Bitmap_Player::load(std::istream&) already gates length >= height*sizeof(uint16_t); the RLE and shadow paths lack an equivalent, and none of the three bound x inside the run loop.

Reproduction (player decoder, minimal)

Calling the buffer-based load with a colored run longer than the row width:

libsiedler2::ArchivItem_Palette pal;
libsiedler2::ArchivItem_Bitmap_Player bmp;
const std::vector<uint16_t> starts(1, 0);          // one row at offset 0
// 0x40 + 10 => colored run of 10 pixels, but the row is only 4 wide
const std::vector<uint8_t> image = {0x4A, 1,1,1,1,1,1,1,1,1,1};
bmp.load(4, image, starts, true, &pal);            // writes x = 0..9 into a width-4 buffer

Before the fix this writes 6 pixels past the row (heap OOB). Any .LST/.BOB with such a run triggers it via the normal load path.

Suggested fix

Bound the write index against width before each pixel run, and guard the position reads, returning WRONG_FORMAT/UNEXPECTED_EOF on malformed data. The checks are no-ops on well-formed rows (a valid run never exceeds the width), so they don't affect loading of genuine S2 assets.

I have a patch on a local branch that does exactly this and adds a regression test (PlayerBitmapRejectsOverlongRun in tests/testBmp.cpp). Verified: the full existing test suite still passes, real S2 Gold assets still load, and the crafted overlong run above now returns WRONG_FORMAT instead of writing OOB. Happy to open a PR if useful.

[Flamefire]

Happy to open a PR if useful.

Please do so and let's discuss there. Looks like an easy fix