Fix Gazebo crash in building editor when adding door or window (#2276)

[jensanjo]

BuildingMaker::DetachFromParent: break loop after DetachAllChildren.
Prevents memory corruption by iterating over attachmentMap while removing elements.

How to reproduce

• Open the building editor
• Add a wall
• Add a window or door to the wall: drag the item into the wall, then continue to drag it outside the wall
• Gazebo crashes

The cause

Backtrace from gdb indicates a segmentation fault during BuildingMaker::DetachFromParent.

The attachmentMap is a map<string, vector<string>>> , for each wall (e.g. 'Wall_0') it holds the attached doors and windows (e.g. 'Door_0'). When adding for instance a new door to a wall, the item is added to the attachmentMap when the cursor enters the wall. When the cursor moves out of the wall, the item must be detached from the wall.
When the item to be detached is the last attached item for the wall, the wall itself must be removed from the attachmentMap. That is done by the call to DetachAllChildren.

The problem is that after DetachAllChildren removes the parent wall from the attachmentMap, DetachFromParent keeps iterating. Iterating over a collection while deleting elements is bad.

My fix
Since a door or window can have only one parent, it makes sense to quit the loop after we have removed the childless parent from the map, by adding a break after DetachAllChildren.

void BuildingMaker::DetachFromParent(const std::string &_child)
{
  for (auto &parentManip : this->dataPtr->attachmentMap)
  {
    parentManip.second.erase(std::remove(parentManip.second.begin(),
        parentManip.second.end(), _child), parentManip.second.end());

    if (parentManip.second.empty()) {
      this->DetachAllChildren(parentManip.first);
      break;
    }
  }
}

[iche033]

thanks for the fix. I had tried to reproduce this issue some time ago but still hasn't been able to:

but what you described makes sense and the fix looks fine.

[saad-salama]

Thanks man thats help me