Fix spritesheet size in load_data_file()

[jagiella]

solves #4606

[Copilot]

Pull request overview

Fixes a Linux crash when loading spritesheets by adjusting how per-sprite data sizes are computed in load_data_file().

Changes:

• Adjust sprite data size calculation when slicing the spritesheet data buffer.

Comments suppressed due to low confidence (1)

src/spritesheet.cpp:76

• Subtracting 1 from the computed sprite size fixes the debug-assert by avoiding &buffer[data_size], but it also truncates every sprite by one byte and can underflow (e.g., if two offsets are equal, or if offset == offsets[i+1].first), producing a huge size and OOB reads. A safer fix is to keep size = offsets[i+1].first - offset and avoid operator[] for the end iterator by using pointer arithmetic on buffer.data() (begin = data()+offset, end = data()+offsets[i+1].first), optionally with a sanity check that offsets[i+1].first >= offset && offsets[i+1].first <= data_size.

        const auto size = offsets[i + 1].first - offset - 1;
        const auto sprite_idx = offsets[i].second;
        auto & sprite = sheet.sprites[sprite_idx];
        auto & data = sheet.data[sprite_idx];
        data = std::move(std::vector<unsigned char>(&buffer[offset], &buffer[offset + size]));

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[xtremeqg]

Like Copilot says, it could use a check to make sure size doesn't go negative but in itself the current change will fix the off-by-one error.

[xtremeqg]

Took a closer look at the original code after getting some sleep and it appears correct. The stack trace in the linked issue doesn't show function names (keeperfx.map is likely missing) so its hard to tell exactly where the problem occurred.

As far as I am aware GCC doesn't have bounds checking enabled by default and none of us have experienced a crash in this particular bit of code. Are you able to attach a debugger and provide the exact stack as well as relevant values?

The force-pushed changes look a bit cleaner, but they do not appear to make any functional changes.

[jagiella]

@xtremeqg

I am on ubuntu 26.04
with g++-15

$ g++ --version
g++ (Ubuntu 15.2.0-12ubuntu1) 15.2.0

The debug output of valgrind is:

$ valgrind ~/keeperfx/build/src/keeperfx
==418187== Memcheck, a memory error detector
==418187== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==418187== Using Valgrind-3.26.0 and LibVEX; rerun with -h for copyright info
==418187== Command: /home/user/keeperfx/build/src/keeperfx
==418187== 
/usr/include/c++/15/bits/stl_vector.h:1263: constexpr std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = unsigned char; _Alloc = std::allocator<unsigned char>; reference = unsigned char&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
KeeperFX fatal signal received
Signal: SIGABRT
/home/user/keeperfx/build/src/keeperfx(+0x10752b) [0x410752b]
/home/user/keeperfx/build/src/keeperfx(+0x10795c) [0x410795c]
/usr/lib/x86_64-linux-gnu/libc.so.6(+0x45f30) [0x1100af30]
/usr/lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x11d) [0x1106afad]
/usr/lib/x86_64-linux-gnu/libc.so.6(gsignal+0x1e) [0x1100adfe]
/usr/lib/x86_64-linux-gnu/libc.so.6(abort+0x27) [0x10fed888]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(+0xae756) [0x10ca8756]
/home/user/keeperfx/build/src/keeperfx(+0x33d4e6) [0x433d4e6]
/home/user/keeperfx/build/src/keeperfx(+0x36b5c6) [0x436b5c6]
/home/user/keeperfx/build/src/keeperfx(+0x36b810) [0x436b810]
/home/user/keeperfx/build/src/keeperfx(+0x31e1a0) [0x431e1a0]
/home/user/keeperfx/build/src/keeperfx(+0x35efc4) [0x435efc4]
/home/user/keeperfx/build/src/keeperfx(+0x35f616) [0x435f616]
/home/user/keeperfx/build/src/keeperfx(+0x367965) [0x4367965]
/home/user/keeperfx/build/src/keeperfx(+0x367ae5) [0x4367ae5]
/home/user/keeperfx/build/src/keeperfx(+0x35c46a) [0x435c46a]
==418187== 
==418187== Process terminating with default action of signal 6 (SIGABRT)
==418187==    at 0x1106AFAD: __pthread_kill_implementation (pthread_kill.c:44)
==418187==    by 0x1106AFAD: __pthread_kill_internal (pthread_kill.c:89)
==418187==    by 0x1106AFAD: pthread_kill@@GLIBC_2.34 (pthread_kill.c:100)
==418187==    by 0x1100ADFD: raise (raise.c:26)
==418187==    by 0x10FED887: abort (abort.c:77)
==418187==    by 0x10CA8755: std::__glibcxx_assert_fail(char const*, int, char const*, char const*) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.35)
==418187==    by 0x433D4E5: std::vector<unsigned char, std::allocator<unsigned char> >::operator[](unsigned long) (stl_vector.h:1263)
==418187==    by 0x436B5C5: (anonymous namespace)::load_data_file(TbSpriteSheet&, std::vector<std::pair<unsigned int, unsigned long>, std::allocator<std::pair<unsigned int, unsigned long> > >&, char const*) (spritesheet.cpp:76)
==418187==    by 0x436B80F: load_spritesheet (spritesheet.cpp:100)
==418187==    by 0x431E19F: load_pointer_file (vidmode.c:270)
==418187==    by 0x435EFC3: initial_setup (main.cpp:955)
==418187==    by 0x435F615: setup_game (main.cpp:1151)
==418187==    by 0x4367964: LbBullfrogMain (main.cpp:4389)
==418187==    by 0x4367AE4: kfxmain (main.cpp:4452)
==418187== 
==418187== HEAP SUMMARY:
==418187==     in use at exit: 105,173,974 bytes in 39,437 blocks
==418187==   total heap usage: 164,272 allocs, 124,835 frees, 235,022,142 bytes allocated
==418187== 
==418187== LEAK SUMMARY:
==418187==    definitely lost: 120 bytes in 5 blocks
==418187==    indirectly lost: 59,738 bytes in 10 blocks
==418187==      possibly lost: 11,171,233 bytes in 4,727 blocks
==418187==    still reachable: 93,941,035 bytes in 34,674 blocks
==418187==                       of which reachable via heuristic:
==418187==                         newarray           : 7,856 bytes in 33 blocks
==418187==         suppressed: 0 bytes in 0 blocks
==418187== Rerun with --leak-check=full to see details of leaked memory
==418187== 
==418187== For lists of detected and suppressed errors, rerun with: -s
==418187== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Abgebrochen                (Speicherabzug geschrieben) valgrind ~/keeperfx/build/src/keeperfx

It happens during the processing of the last sprite, when &buffer[offset + size] actually triggers the out of bounds assertion.

[jagiella]

Took a closer look at the original code after getting some sleep and it appears correct. The stack trace in the linked issue doesn't show function names (keeperfx.map is likely missing) so its hard to tell exactly where the problem occurred.

As far as I am aware GCC doesn't have bounds checking enabled by default and none of us have experienced a crash in this particular bit of code. Are you able to attach a debugger and provide the exact stack as well as relevant values?

The force-pushed changes look a bit cleaner, but they do not appear to make any functional changes.

I agree with you that the functional changes are zero. But algorithmically it changes

• from buffer[last] being a pointer arithmetics
• to buffer.begin() + last being an iterator arithmetics

Given last==buffer.size(), the first (according to gcc) will lead to array overflow, while the second equals to buffer.end() (hypotheses). 😄

[xtremeqg]

Okay I see, so it is the bounds check itself that is causing the crash. The byte past the input buffer is never actually accessed thus there is no crash under normal conditions.

from buffer[last] being a pointer arithmetics

This is the subscript operator where logically one would implement bounds checking as seen in the stack trace.

to buffer.begin() + last being an iterator arithmetics

This seems to bypass the bound check by advancing the iterator instead (which is likely just a pointer as value_type is just a POD).

The end result is the same, yes (a pointer one byte past the input buffer).

I think this change can be merged as-is.