[One] Server crashes on Windows + reason/workaround

[evilatwow]

I recently updated to a mangos-one server r1338, and noticed the server crashes every time I shut it down. Though it seems to run fine otherwise, I still find this annoying so I checked it out. The revision isn't important as the latest code still has the problem. And it also happened with the default empty databases, and when just shutting down the server immediately after it's started up (so no need to enter the world first or something).

The crash logs gave me a first hint:

Exception code: C0000005 ACCESS_VIOLATION

...

Call stack:

Address Frame Function SourceFile

00427D6F 00000000 std::_Tree<std::_Tset_traits<unsigned int,std::less<unsigned int>,std::allocator<unsigned int>,0> >::_Eqrange+F

004188C2 00000000 std::_Tree<std::_Tset_traits<unsigned int,std::less<unsigned int>,std::allocator<unsigned int>,0> >::erase+22

00797714 00000000 BattleGround::~BattleGround+154

00850568 00000000 BattleGroundRL::~BattleGroundRL+18

0069454F 00000000 BattleGroundRL::`scalar deleting destructor'+F

0069207C 00000000 BattleGroundMgr::DeleteAllBattleGrounds+AC

0046381D 00000000 WorldRunnable::run+FD

00930219 00000000 ACE_Based::Thread::ThreadTask+19

01022E34 00000000 ?invoke@ACE_OS_Thread_Adapter@@UAEKXZ+74

78543433 00000000 _endthreadex+44

785434C7 00000000 _endthreadex+D8

7C57B388 00000000 lstrcmpiW+B7

There's not much info here since it's a release build, but I figured it out nevertheless. The erase call that happens from the BattleGround destructor is actually this code:

sBattleGroundMgr.DeleteClientVisibleInstanceId(GetTypeID(), GetBracketId(), GetClientInstanceID());

That code is doing the erase:

void DeleteClientVisibleInstanceId(BattleGroundTypeId bgTypeId, BattleGroundBracketId bracket_id, uint32 clientInstanceID)
{
   m_ClientBattleGroundIds[bgTypeId][bracket_id].erase(clientInstanceID);
}

Upon inspection of some relevant code, I think this is what happens:

1. BattleGround.h includes an enum:

enum BattleGroundBracketId
{
   BG_BRACKET_ID_FIRST          = 0,
   BG_BRACKET_ID_LAST           = 6,

   MAX_BATTLEGROUND_BRACKETS   = 7
};

2. The BattleGroundMgr class has the following member:

ClientBattleGroundIdSet m_ClientBattleGroundIds[MAX_BATTLEGROUND_TYPE_ID][MAX_BATTLEGROUND_BRACKETS];

3. Battlegrounds are constructed while starting up the server (or at least 'bg templates' are).

4. The constructor, BattleGround::BattleGround(), sets the m_BracketId member to MAX_BATTLEGROUND_BRACKETS.

5. When shutting down the server, BattleGroundMgr::DeleteAllBattleGrounds() gets called, which will delete the battlegrounds.

6. The BattleGround destructor will call DeleteClientVisibleInstanceId, passing it MAX_BATTLEGROUND_BRACKETS as bracket ID.

7. The DeleteClientVisibleInstanceId tries to call erase on m_ClientBattleGroundIds[<type id>][MAX_BATTLEGROUND_BRACKETS], but that one is outside of the valid range of the array, since in C/C++ array indexes start at 0, so this crashes.

As a temporarily workaround I've changed the BattleGroundMgr member as follows:

ClientBattleGroundIdSet m_ClientBattleGroundIds[MAX_BATTLEGROUND_TYPE_ID][MAX_BATTLEGROUND_BRACKETS+1];

As can be expected, this fixed my crash.

I've seen that the BattleGround constructor mentions "// use as mark bg template" in the comments when setting the m_BracketId member to MAX_BATTLEGROUND_BRACKETS, and I'm not sure what the bg template stuff is all about. So I doubt my fix is a proper one (which is why I used the word workaround). The main branch of MaNGOS seems to set this member to BG_BRACKET_ID_FIRST. This would also fix the problem, but there is no mention of the "bg template" stuff any more, so I'm not sure if that's a good idea or what difference it could make.

So I'll leave the real fix to the experts, and hope my findings provide enough info

[stfx]

Thanks for your research. I spent a few hours trying to find the correct fix and came up with the following:

https://gist.github.com/1299877

This is what I got so far:

At first the template bgs are being initialized (1 template for each bg map type).

When initializing a bg on One and Zero we use MAX_BATTLEGROUND_BRACKETS for default m_BracketId which is used as a marker for us to see if that bg is a template or not.

When removing the bg templates, DeleteClientVisibleInstanceId is being called and tries, as you already found out, to erase an entry which an index out of the boundary of the array.

Now setting m_BracketId to BG_BRACKET_ID_FIRST as it is done in the master is probably not such a good idea as on master the bracket id actually is a pointer to dbc structure associated with bg copy, in One on the other hand this is the id itself. Meaning that 0 (BG_BRACKET_ID_FIRST) is a bracket on One but on master it actually is none, because first id is 1 in PvpDifficulty.dbc - not sure about that though. See this commit for more details: https://github.com/mangos/mangos/commit/b9712d36d2c285be14cf7bb9bd5cc1f4c9c58cf3

So yeah please try the patch at the top of this post, thanks

[DaC]

Changed the uint32 to BattleGroundBracketId as return type of GetBracketId()

https://gist.github.com/1301175

...and it works, clean shutdown.

PS: thats no windows problem

[evilatwow]

Yeah, I figured that there was an important difference between the bracket IDs in Zero/One and Master. Good to see that confirmed; so I agree with your fix

PS: thats no windows problem

No, it's not, it's a problem in general

But for whatever reason, this crash was only showing on my Windows box and not on the Linux machine. Must have been lucky there

[stfx]

Meaning that 0 (BG_BRACKET_ID_FIRST) is a bracket on One but on master it actually is none, because first id is 1 in PvpDifficulty.dbc - not sure about that though.

To correct myself this is actually not true for master. On master a bracket_id of 0 is also an actual bracket because the ids in PvpDifficulty.dbc are between 0 and 15. Therefore setting the initial m_BracketId = BG_BRACKET_ID_FIRST is incorrect!

This is the patch for master: https://gist.github.com/1301721

The patch for One will be the same after its been added to master repo.

[VladimirMangos]

at master bracket id not used independent from bracketEntry as i remember. But i agree that use BG_BRACKET_ID_FIRST by default not best way.

[stfx]

Pushed to One in [s1431]. Still waiting for https://gist.github.com/1301721 to be added to master