Crash: SendSurroundingChunkPreLoaded

[John Adams]

I fixed the crash by simply checking if "info" was valid; seems to have stopped the world crashes. But what I want to know is why if we have no chunks "preloading", why is this even being called? Yet another random thing thrown into HandleClientAuthConfirm unnecessarily.

Stack

Code: Select all

>	WorldServer.exe!ChunkServer::SendSurroundingChunkPreLoaded(std::shared_ptr<Client> & client) Line 2057	C++
 	WorldServer.exe!ChunkServer::HandleClientAuthConfirm(std::shared_ptr<Client> & client, PacketStruct * packet_struct) Line 1298	C++
 	WorldServer.exe!ChunkServer::ProcessPackets() Line 313	C++
 	WorldServer.exe!ChunkPacketThread(void * data) Line 120	C++
 	WorldServer.exe!ThreadRun(void * arg) Line 77	C++
 	WorldServer.exe!_callthreadstart() Line 255	C
 	WorldServer.exe!_threadstart(void * ptd) Line 239	C
 	kernel32.dll!7716337a()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77c492e2()	Unknown
 	ntdll.dll!77c492b5()	Unknown

"info" is null, and there is no check. Again.

[Blackstorm]

SurroundingChunkPreLoaded is more like SurroundingChunkAvailable

Each time the client receive the 8 surrounding chunk status. Locked (op 51) or available (op 96).

If this functions has crash the server, maybe the same issue is also in the function SurroundingChunkInactives().

[Xinux]

Never seen the surroundingchunkinactive cause a crash.

[John Adams]

Tested vigorously, SendSurroundingChunkInactives (the original function) has no problems at all.

But the new(er) SendSurroundingChunkPreloaded() crashes every time, and it's likely the chunk I happen to be in.

Looking at the code (and I cannot believe Lokked did this originally)

Code: Select all

void ChunkServer::SendSurroundingChunkPreLoaded(shared_ptr<Client> &client) {    int i, j;    ChunkInfo *info = nullptr;    for (i = -1; i != 2; ++i) {        for (j = -1; j != 2; ++j) {            info = chunk_info_list.GetChunkInfoByCoords(this->coord_x + i, this->coord_y + j);            vector<uint32_t> chunklist = chunk_info_list.GetLoadedChunkIDs();            for(uint32_t itr : chunklist) {                if ((info->chunk_id == itr)&&(this->chunk_id != itr)&&(info->is_active == 1)) {                    PacketStruct *out;                    if ((out = packet_struct_list.GetPacketStruct2Server("WS_ChunkPreload")) == NULL)                        continue; 

[quote]this->coord_x + 1, this->coord_y + 1[/quote]
This blows my mind that anyone would think that's the right way to do this. What if I'm on the edge of a map, and the +1 is not there? What happens? What if the chunk I am on is -27 and you +1 to see the next one, but it's supposed to be -28? And, why was "info" never checked for null?

I think I understand the desire for SurroundingChunkPreLoaded - but nothing is preloaded, yet we're looping anyway. There are so many things wrong with this function, that's just off the top of my head.

Someone will need to refactor this (and Review the ChunksInactive function), after determining if it's even needed at all.