azerothcore / azerothcore-wotlk

Complete Open Source and Modular solution for MMO
http://www.azerothcore.org
GNU Affero General Public License v3.0
6.57k stars 2.63k forks source link

Core/Maps entry assert #10259

Closed Annamaria-CC closed 2 years ago

Annamaria-CC commented 2 years ago

Rev: 124ea8a1e7d0

Modules; mod-anticheat mod-autobalance mod-cfbg mod-chromie-xp mod-desertion-warnings mod-duel-reset mod-ip-tracker mod-eluna-lua-engine mod-low-level-arena mod-progression-system mod-pvp-titles mod-pvpstats-announcer mod-queue-list-cache mod-rdf-expansion mod-server-auto-shutdown mod-spellsystem-extended

Operating system Unix

See pastebin https://pastebin.com/1WzLnT11

Kitzunu commented 2 years ago

https://github.com/azerothcore/azerothcore-wotlk/blob/b5075e9a0a203bb0ee44b232a09e4f5452a39c63/src/server/game/Maps/MapMgr.cpp#L79-L83

Are you using any custom maps or trying to access maps that don't exist in DBC?

Nyeriah commented 2 years ago

andler=0xcfd500 <go_commandscript::HandleGoZoneXYCommand(ChatHandler*, float, floa

Looks like someone tried to port to an obscure place

Annamaria-CC commented 2 years ago

No custom maps. Not sure about the second question,

if so, shouldn't result in a crash I suppose?

Kitzunu commented 2 years ago

you can see in the code above that it will assert if the map entry can't be found in dbc

Nyeriah commented 2 years ago

The GoZoneXY command needs a check to see if the map theyre porting to is valid

unfortunately it’s a ptr crash so it’s impossible to tell what they tried to do

Kitzunu commented 2 years ago

The GoZoneXY command needs a check to see if the map theyre porting to is valid

Well that shouldn't be necessary as you don't have an argument to provide map ID. Only for zone id, and if the zone id doesn't exist it will assert within that command

and that is what is throwing me off at the moment because we have an ASSERT for zoneid, and under we call the create map

https://github.com/azerothcore/azerothcore-wotlk/blob/b24fd8173439db21abf736dae9482710d4c418f8/src/server/scripts/Commands/cs_go.cpp#L278-L282

So the zone provided is "valid" but has no map?

Kitzunu commented 2 years ago

and they didnt even provide a zoneid? only coordinates if I read this correctly

0x0000000000cfd6e0 in go_commandscript::HandleGoZoneXYCommand (handler=0x7ffec063cc38, x=40, y=40, areaIdArg=std::optional<Acore::ChatCommands::Variant<Acore::ChatCommands::Hyperlink<Acore::Hyperlinks::LinkTags::area>, unsigned int>> 

So this should not be possible

Kitzunu commented 2 years ago
0  0x0000000001fe08d3 in Acore::Assert (file=0x23cd708 "/root/env/azerothcore-wotlk/src/server/game/Maps/MapMgr.cpp", line=83, function=0x23cd744 "CreateBaseMap", debugInfo=Python Exception <class 'gdb.error'> There is no member named _M_dataplus.: 
, message=0x23d7a3a "entry") at /root/env/azerothcore-wotlk/src/common/Debugging/Errors.cpp:64
        formattedMessage = <incomplete type>
#1  0x000000000185ac8a in MapMgr::CreateBaseMap (this=0x2e732d8 <MapMgr::instance()::instance>, id=150) at /root/env/azerothcore-wotlk/src/server/game/Maps/MapMgr.cpp:83
        entry = 0x0
        guard = {_M_device = @0x2e732d8}
        map = 0x0
#2  0x0000000000cfd6e0 in go_commandscript::HandleGoZoneXYCommand (handler=0x7ffec063cc38, x=40, y=40, areaIdArg=std::optional<Acore::ChatCommands::Variant<Acore::ChatCommands::Hyperlink<Acore::Hyperlinks::LinkTags::area>, unsigned int>> = {...}) at /root/env/azerothcore-wotlk/src/server/scripts/Commands/cs_go.cpp:282
        player = 0x7fa6d7dd88c0
        areaId = 676
        areaEntry = 0x7fa725371694
        zoneEntry = 0x7fa725371694
        map = 0x54ae98 <std::__detail::__variant::_Move_ctor_base<false, std::monostate, std::basic_string_view<char, std::char_traits<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::~_Move_ctor_base()+24>
        z = 0
Nyeriah commented 2 years ago

That assert inside that gm command makes no sense and needs to go away though. The server shouldn't crash if someone decides to try some random value when teleporting

Kitzunu commented 2 years ago

it is for the zone id if you provide it.

Kitzunu commented 2 years ago

But, that assert should have hit imo and not the one in createmap. because how can a zone exist but a map doesn't?

Nyeriah commented 2 years ago

ASSERTs are to be used in very specific situations, code that will irreparably break the core, points of no return that only shutting the entire thing down would make sense. A GM command can just return if something is wrong and print an error instead of shutting the entire server down and no harm will be done.

If I read that thing carefully, the zone falls back if it's null and uses the current area's parent zone, so it passes the zone check, even if the map creation fails a few lines below.

Kitzunu commented 2 years ago

yea but I am still stuck on how we can have a valid zone but not a valid map

acidmanifesto commented 2 years ago

not sure if relevant but azerothcore seems to be the only core i see with this if statement https://github.com/azerothcore/azerothcore-wotlk/blob/b5075e9a0a203bb0ee44b232a09e4f5452a39c63/src/server/game/Maps/MapMgr.cpp#L80 where other cores do not. Trinitycore 335 https://github.com/TrinityCore/TrinityCore/blob/fd7f48545ce76740065860c0963194c2a9e3e0e2/src/server/game/Maps/MapManager.cpp#L74-L76

Cata Preservation, Trinitycore Master and skyfire has it as if there is !map https://github.com/The-Cataclysm-Preservation-Project/TrinityCore/blob/803bd2c79b058be1128f38e15090985fb59e2f78/src/server/game/Maps/MapManager.cpp#L79-L83 https://github.com/TrinityCore/TrinityCore/blob/aa742c8752d35f3d1a216a4a8ac6a4987cc4da7e/src/server/game/Maps/MapManager.cpp#L79-L82 https://github.com/ProjectSkyfire/SkyFire_548/blob/781ce78b7c43b1e212c1663dffe6f9f1543362a5/src/server/game/Maps/MapManager.cpp#L103-L106

I guess what i am saying, are we certain we need to check with it being a nullptr and not a !map?

UltraNix commented 2 years ago

Removing an assert is not a fix. The only problem is the arg provided here. go zonexy 40 40 676. Area/Zone 676 ("Outland") is invalid. In my opinin nothing is worng here in the code.

UltraNix commented 2 years ago

Close it as it's not a core problem. And an assert works great in situations like these.