1. We have added a Gift Upgrades feature that allows you to gift an account upgrade to another member, just in time for the holiday season. You can see the gift option when going to the Account Upgrades screen, or on any user profile screen.
    Dismiss Notice

Pointer Invalidation issues

Discussion in 'Bugs and Crashes' started by alberts2, Jul 19, 2019.

  1. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    Code:
    CLLNode<IDInfo>* pUnitNode = pPlot->headUnitNode();
    
                        while (pUnitNode != NULL)
                        {
                            CvUnit* pLoopUnit = ::getUnit(pUnitNode->m_data);
                            pUnitNode = pPlot->nextUnitNode(pUnitNode);
    
                            if (pLoopUnit->getTeam() == getTeam())
                            {
                                Code that modifies the list of units on a PLOT!
                            }
                        }
    CivIV and C2C have issues with pointer invalidation, this happens in code like the above segment.

    Every time the list of units on a plot or in a selectiongroup is modified after calling nextUnitNode it's possible that the pUnitNode pointer becomes invalid. This is a programming error that is becomming a problem especially with Windows 10 because newer windows versions have more agressive memory management.

    C2C already has all kinds of code to avoid crashes because of this issues but most of them are flawed i post some examples later.
     
    Last edited: Jul 19, 2019
    raxo2222 and Anq like this.
  2. raxo2222

    raxo2222 Warlord

    Joined:
    Jun 10, 2011
    Messages:
    5,744
    Location:
    Poland
    I wonder if this is cause of crashes, that happen on Windows 10 but not on Windows 7.
     
  3. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    Here a example from a another mod a few years ago.
     
  4. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
     
  5. Anq

    Anq Chieftain

    Joined:
    Apr 14, 2019
    Messages:
    350
    Gender:
    Male
    Location:
    Anser (geese) HQ
    In this case, I think one should call pPlot->removeUnit(pLoopUnit) before the kill to prevent pointer invalidation, is this correct?
    So the general pattern here is they are dealing with doubly linked lists. Deleting a pointer whilst traversing a list creates potential pointer invalidation, if one tries to jump back to a previous node.

    Pointer access violation happens as soon as the list is being deleted. This is because you cannot delete a pointer twice.

    https://forums.civfanatics.com/thre...f-february-2018.629241/page-196#post-15493621
    This earlier bug report shows the program crashed at ~CvTalkingHeadMessage()
     
    Last edited: Jul 19, 2019
  6. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    Going to the next node in a loop is not possible if the next node has been changed or removed.
     
  7. Anq

    Anq Chieftain

    Joined:
    Apr 14, 2019
    Messages:
    350
    Gender:
    Male
    Location:
    Anser (geese) HQ
    Although m_data is deleted, the m_prev and m_next pointers still point to the other nodes, so I think the while loop may continue, but the game will crash at the destruction of the list.
    Some time? I haven't study that code fully.
     
  8. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    Yes and in the current coding the IDInfo m_data is being read after it's been removed and the memory has possibly been overwritten with other data at that time it is being read.

    That information is invalid as soon as the node has been deleted.
     
  9. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    The other issue is that C2C has lot's of flawed code to move around this issue in various places.
    Here a example
    Code:
                std::set<int> movedUnitIds;
    
                while (true)
                {
                    pUnitNode = pOldPlot->headUnitNode();
                    pLoopUnit = NULL;
    
                    while (pUnitNode != NULL)
                    {
                        if (movedUnitIds.empty() || movedUnitIds.find(pUnitNode->m_data.iID) == movedUnitIds.end())
                        {
                            pLoopUnit = ::getUnit(pUnitNode->m_data);
    
                            FAssertMsg(pLoopUnit != NULL, "NULL unit reached in the selection group");
    
                            if (pLoopUnit != NULL)
                                break;
                        }
    
                        pUnitNode = pOldPlot->nextUnitNode(pUnitNode);
                    }
    
                    if (pLoopUnit == NULL)
                        break;
                   
                    movedUnitIds.insert(pLoopUnit->getID());
    
                    if (pLoopUnit->getTransportUnit() == this)
                    {
                        //GC.getGameINLINE().logOOSSpecial(22, pLoopUnit->getID(), iX, iY);
                        pLoopUnit->setXY(iX, iY, bGroup, false);
                    }
                }
    This is wrong because only the unit ID's are stored in a std::set and units of different players can have the same ID. The complete IDInfo must be used here.
    It's also inefficient because of std::set is used.

    I think something like this could work much better but i'am not yet sure if it's a proper solution.
    Code:
                pUnitNode = pOldPlot->headUnitNode();
                std::vector<IDInfo> cargoUnits;
                while (pUnitNode != NULL)
                {
                    pLoopUnit = ::getUnit(pUnitNode->m_data);
                    pUnitNode = pOldPlot->nextUnitNode(pUnitNode);
    
                    if (pLoopUnit->getTransportUnit() == this)
                    {
                        //GC.getGameINLINE().logOOSSpecial(22, pLoopUnit->getID(), iX, iY);
                        cargoUnits.push_back(pLoopUnit->getIDInfo());
                    }
                }
                for(std::vector<IDInfo>::const_iterator it = cargoUnits.begin(), end = cargoUnits.end(); it != end; ++it)
                {
                    pLoopUnit = ::getUnit(*it);
                    if (pLoopUnit != NULL)
                    {
                        pLoopUnit->setXY(iX, iY, bGroup, false);
                    }
                }
     
    Last edited: Jul 19, 2019
    raxo2222 likes this.
  10. Thunderbrd

    Thunderbrd C2C War Dog

    Joined:
    Jan 2, 2010
    Messages:
    25,686
    Gender:
    Male
    Location:
    Las Vegas
    If I'm not mistaken, this was the problem that I was copying the layout of some coding to try to resolve that was causing the std::find on a set inefficiency. I'm just now hearing about and starting to really learn what a set is beyond just seeing code and having a clue how to work with it based on my understanding of vectors. Only in the last month did I really even come to grasp that there WAS such a thing as a set, as distinct from a vector, array, or map, thanks to these discussions that this bug brought up. I have a lot to learn here... all the more than I thought.
     
  11. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    That's another problem.

    std::set is a ordered Container every time you insert something it must search the location where the new item must be located to keep that order. This takes time and std::set should only be used if it's necessary to have a collection ordered otherwise a std::vector could be used instead.
     
  12. alberts2

    alberts2 Chieftain

    Joined:
    Aug 16, 2012
    Messages:
    1,898
    Gender:
    Male
    Location:
    Germany
    @Thunderbrd

    At least this change should be made before the release the rest could be looked at later.
     
    KaTiON_PT likes this.
  13. Thunderbrd

    Thunderbrd C2C War Dog

    Joined:
    Jan 2, 2010
    Messages:
    25,686
    Gender:
    Male
    Location:
    Las Vegas
    We'll have to address this after release.

    You said:
    Which tells me there's a lot of these spots to look at and not all of them have been identified here, and:
    From which, I gather, crashes are infrequent, though can happen, and we're talking about an efficiency issue (not exactly 100% critical since we can always improve efficiency in many ways)
    and most importantly:
    If you aren't dead sure it's the right way to go with the fix, I don't want to insert something untested into the code anywhere that could cause some kind of unexpected issues worse than what we have now. I'm not saying this is not a high priority matter - I totally get it because you and I have been encountering these problems for a long time and they are confounding as hell. I'm saying I'd prefer to go with the devil as we know it for the release than the one we don't yet know, applied in only one of many potential places it should be. We can always find very good reason to hold up releases. Hopefully we can get these fixes implemented soon after release and they work great and become a big part of why we'll have a good v40 release and for those who encounter the problem we can simply say, yeah we dealt with that already so get the latest revision and start a new game and go from there. We run into these bugs now and then but not at such frequency that its worthwhile to introduce an untested element just before release. I believe you're right that it's a good fix but man... if for any reason it isn't and we only find out from feedback after we release this thing, that'd be a nightmare.
     

Share This Page