fix ShopEx Unknown Crash

[LordZiege 369]

Hello,

i got a weird crash with AddressSanitizer enabled on Windows, so im here to share a fix. (Not sure if its 100% correct, but fixxed the unknown crash ASAN reported.)

Quote

AddressSanitizer: unknown-crash on address....

in shopex.cpp search:

memcpy(pack_tab.name, shop_tab.name.c_str(), SHOP_TAB_NAME_MAX);

and replace with:

memcpy(pack_tab.name, shop_tab.name.c_str(), sizeof(shop_tab.name));

 

[Mind Rapist 897]

Before reading this I had no idea what shopEx even is to be honest but when I saw the word "crash" in the title I had to dig into it.

I audited the issue and the recommended solution and apparently shopEx seems to be a multi-tab shop configuration through a txt file (shop_table_ex.txt). Not a lot of modern SF systems use this method (at least the ones I came across) so this issue is as the automated agent described a "dormant landmine", though apparently the issue is very real.

I would like to share the findings of my automated audit for anyone that may be interested, I know that a lot of people are against the use of AI and agents and I have by no means tested this!, I'm just sharing what I found for concept and for the sake of discussion, what truly matters here is what the more experienced human devs have to say about this as well as people that tested this solution.

 

Quote

shopEx bug

File: game/shopEx.cpp:77-79

Two bugs in three lines:

1. Over-read on the source.

memcpy(pack_tab.name, shop_tab.name.c_str(), SHOP_TAB_NAME_MAX)

always reads 32 bytes from a std::string that's usually shorter. Reads past the string's allocated content → ASAN flags heap-buffer-overflow; in non-sanitizer builds it's silent UB.

2. Uninitialized destination. pack_tab is a stack local, never zeroed. The bytes after the copied name (and any struct padding) keep whatever was on the stack — then the whole struct is sent to the client over the network. Info leak of server stack data on every shopEx open.

Why the "sizeof(shop_tab.name)" fix is wrong: that's sizeof(std::string) (the object, ~40 bytes on MSVC), not the content length. It silences ASAN by coincidence on some STLs but doesn't fix the semantics — and it can write std::string internal fields (capacity/allocator state) into the packet instead.

Current exposure: zero — no shop_table_ex.txt exists in our deployment, so CShopEx is never instantiated. Dormant landmine that activates the moment anyone enables multi-tab NPC shops.

Recommended fix (~3 lines):

TPacketGCShopStartEx::TSubPacketShopTab pack_tab;
memset(&pack_tab, 0, sizeof(pack_tab)); // close info leak
pack_tab.coin_type = shop_tab.coinType;

const size_t n = std::min(shop_tab.name.size(), (size_t)(SHOP_TAB_NAME_MAX - 1));
memcpy(pack_tab.name, shop_tab.name.data(), n); // bounded read
// pack_tab.name[n] already \0 from memset

Fixes both bugs. The existing length validation in shop_manager.cpp:374 stays as defense-in-depth.

Status: parked. Revisit if/when multi-tab NPC shops get enabled, or fold into a routine memory-safety pass.

Again, I am posting these findings simply for discussion, for more experienced devs this may be a no-brainer but for the more novice ones this may be a chance to explore and familiarize better with their codebases while experimenting and potentially solving an issue the right way.

I will test for these issues myself to experience them with my own eyes and I will also test the recommended fix by my audit, let me know if you would like me to post my experience here after I'm done testing and experimenting.

[Andu x Regina 1]

strlcpy was made for such thing

Spoiler

memset(&pack_tab, 0, sizeof(pack_tab));

strlcpy(pack_tab.name, shop_tab.name.c_str(), sizeof(pack_tab.name));