CreateItem in item_manager.cpp - SetCount checked after the item is already registered

[Larry Watterson 484]

Found this while going through CreateItem. Near the end of the function:

item->SetVID(++m_dwVIDCount);

if (bSkipSave == false)
    m_VIDMap.emplace(item->GetVID(), item);

if (item->GetID() != 0 && bSkipSave == false)
    m_map_pkItemByID.emplace(item->GetID(), item);

if (!item->SetCount(count)) // LOL
    return NULL;

The item gets a VID and gets registered(emplace) in both maps before "SetCount" is even checked. If "SetCount" fails, the function just returns NULL but the item is still in those two maps. The only place that cleans them up is "DestroyItem()", which never gets called here.

Right now this isn't actively broken, because the only path where "SetCount" returns false (count == 0 with no owner) already calls M2_DESTROY_ITEM on itself before returning. But that's the only thing saving you. It's relying entirely on "SetCount" funcs current behavior. Add another false-return case to "SetCount" later without cleaning up after itself, and you get a permanent ghost item stuck in m_VIDMap/m_map_pkItemByID, still reachable through Find()/FindByVID(), never saved, never freed.

Fix is just reordering, check "SetCount" before registering anything:

if (!item->SetCount(count)) [[unlikely]]
    return nullptr;

item->SetVID(++m_dwVIDCount);

if (bSkipSave == false)
    m_VIDMap.emplace(item->GetVID(), item);

if (item->GetID() != 0 && bSkipSave == false)
    m_map_pkItemByID.emplace(item->GetID(), item);

This is a cold path so statement probably never triggered right now, but it could turn into a real problem down the line. Found this reading the code, not from a live crash/dupe report, so test it on your own source before dropping it in(especially if you've touched "SetCount" yourselves).