Azerothcore-wotlk: Crash server when malformed packet (opcode CMSG_AUCTION_LIST_OWNER_ITEMS)

[Helias]

SMALL DESCRIPTION:

The server crash when send a malformed packet with the following opcode CMSG_AUCTION_LIST_OWNER_ITEMS

EXPECTED BLIZZLIKE BEHAVIOUR:

The server should not crash.

CURRENT BEHAVIOUR:

The server crash.

STEPS TO REPRODUCE THE PROBLEM:

1. download WPS.exe from here
2. Run your server, WPS.exe and wow.exe
3. Attach to WPS.exe the process wow.exe choosing the "Target" (on top-right of WPS window)
4. login in game
5. go to WPS and disable/enable (one or twice) the checkbox "Crash"
   
6. a button "Crash" will appear in-game (if not enable/disable again the checkbox), click it to make crash the server.

Thx @SolidMaxtor to report me this issue.

Fixed available here: https://github.com/azerothcore/azerothcore-wotlk/pull/2684

BRANCH(ES):

master

AC HASH/COMMIT:

32dcb3bf63487a8ac703159a2fdc96d3aaad9a6e

OPERATING SYSTEM:

Server: Linux Ubuntu 18.04.3 LTS

I used Windows 10 to send the malformed packet.

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[Helias]

@pklloveyou ignore errors, you can still use it.

[Yehonal]

for posterity:

There were 2 issues related to it, one "hidden wrong behaviour" and the crash itself.

1. deletion of the packet object: the opcode handler used an internal class to handle the receiving and sending of the packet in async way (using Player events manager). However, the WorldSession::Update() deletes the recvPacket pointer as soon as the opcode method ends: https://github.com/azerothcore/azerothcore-wotlk/blob/999d588c37ef0e56d9043f1aa33ad5e9796f74ca/src/server/game/Server/WorldSession.cpp#L362
   That function passed the recvPacket as a reference to the Async class. That causes unpredictable behaviours since, most likely, the memory could be deleted before the async function is executed. That's why, in this cases, the recvPacket object should be cloned/passed by copy instead of reference (&)

2. The crash: the crash was caused by the fact that, in a normal context, the bytebuffer class throw an exception when a packet contains empty/wrong data, this exception is then caught by the WorldSession::Update(). However, the pussywizard async code moved the extraction of the package outside the Update() loop, in this way the Bytebuffer error throwing was not caught and the server crashed.

The fix to both issues was very simple, the extraction of the packet can be done at the same time it's received and within the WorldSession::Update() loop. It's useless and dangerous to do it in async way. The sending for the response instead can be done asynchronously. And that's what we did.

So, please take care before working on Worldsession handlers :)