simgear/props/props.cxx: Avoid possible SEGV e.g. if listener removes itself.

simgear/props/props_test.cxx: test for self-unregister.

SGPropertyNode::removeChangeListener() used to modify the _listeners vector or
even delete it and set _listeners to NULL, if a listener was removed.

This could cause a SEGV if removeChangeListener() was called from
a listener, because listeners are called from methods such as
SGPropertyNode::fireValueChanged() which iterate through _listeners and don't
expect it to be changed to NULL.

The fix here is to keep a track of whether we are iterating through the
_listeners vector; if we are, SGPropertyNode::removeChangeListener() sets the
entry to NULL, instead of removing it.

We have a new internal function forEachListener() which takes
care of the iterating details - incrementing/decrementing the new
_listeners->_num_iterators count and skipping NULL entries. Before returning,
if _num_iterators is now zero it removes any NULL entries in the vector of
listeners.

The nListeners() member is no longer inline, because it needs to ignore NULL
entries in the vector. And _listeners now points to an internally-defined
struct which contains a new _num_iterators as well as the vector of listeners.

Added self-unregister test to simgear/props/props_test.cxx.

simgear/props/props.cxx

@@ -48,6 +48,18 @@ using std::stringstream;
 
 using namespace simgear;
 
+
+struct SGPropertyNodeListeners
+{
+  /* This keeps a count of the current number of nested invocations of
+  forEachListener(). If non-zero, other code higher up the stack is iterating
+  _items[] so for example code must not erase items in the vector. */
+  int _num_iterators = 0;
+  
+  std::vector<SGPropertyChangeListener *> _items;
+};
+
+
 ////////////////////////////////////////////////////////////////////////
 // Local classes.
 ////////////////////////////////////////////////////////////////////////
@@ -968,7 +980,7 @@ SGPropertyNode::~SGPropertyNode ()
 
   if (_listeners) {
     vector<SGPropertyChangeListener*>::iterator it;
-    for (it = _listeners->begin(); it != _listeners->end(); ++it)
+    for (it = _listeners->_items.begin(); it != _listeners->_items.end(); ++it)
       (*it)->unregister_property(this);
     delete _listeners;
   }
@@ -2392,8 +2404,21 @@ SGPropertyNode::addChangeListener (SGPropertyChangeListener * listener,
                                    bool initial)
 {
   if (_listeners == 0)
-    _listeners = new vector<SGPropertyChangeListener*>;
+    _listeners = new SGPropertyNodeListeners;
-  _listeners->push_back(listener);
+
+  /* If there's a nullptr entry (a listener that was unregistered), we
+  overwrite it. This ensures that listeners that routinely unregister+register
+  themselves don't make _listeners->_items grow unnecessarily. Otherwise simply
+  append. */
+  auto it = std::find(_listeners->_items.begin(), _listeners->_items.end(),
+        (SGPropertyChangeListener*) nullptr
+        );
+  if (it == _listeners->_items.end()) {
+    _listeners->_items.push_back(listener);
+  }
+  else {
+    *it = listener;
+  }
   listener->register_property(this);
   if (initial)
     listener->valueChanged(this);
@@ -2405,14 +2430,29 @@ SGPropertyNode::removeChangeListener (SGPropertyChangeListener * listener)
   if (_listeners == 0)
     return;
   vector<SGPropertyChangeListener*>::iterator it =
-    find(_listeners->begin(), _listeners->end(), listener);
+    find(_listeners->_items.begin(), _listeners->_items.end(), listener);
-  if (it != _listeners->end()) {
+  if (it != _listeners->_items.end()) {
-    _listeners->erase(it);
+    if (_listeners->_num_iterators) {
-    listener->unregister_property(this);
+      /* _listeners._items is currently being iterated further up the stack in
-    if (_listeners->empty()) {
+      this thread by one or more nested invocations of forEachListener(), so
-      vector<SGPropertyChangeListener*>* tmp = _listeners;
+      we must be careful to not break these iterators. So we must not delete
-      _listeners = 0;
+      _listeners. And we must not erase this entry from the vector because that
-      delete tmp;
+      could cause the next listener to be missed out.
+
+      So we simply set this entry to nullptr (nullptr items are skipped by
+      forEachListener()). When all invocations of forEachListener() have
+      finished iterating and it is safe to modify things again, it will clean
+      up any nullptr entries in _listeners->_items. */
+      *it = nullptr;
+      listener->unregister_property(this);
+    }
+    else {
+      _listeners->_items.erase(it);
+      listener->unregister_property(this);
+      if (_listeners->_items.empty()) {
+        delete _listeners;
+        _listeners = 0;
+      }
     }
   }
 }
@@ -2461,15 +2501,67 @@ SGPropertyNode::fireChildrenRemovedRecursive()
   }
 }
 
+/* Calls <callback> for each item in _listeners. We are careful to skip nullptr
+entries in _listeners->items[], which can be created if listeners are removed
+while we are iterating. */
+static void forEachListener(
+    SGPropertyNode* node,
+    SGPropertyNodeListeners*& _listeners,
+    std::function<void (SGPropertyChangeListener*)> callback
+    )
+{
+  if (!_listeners) return;
+  
+  _listeners->_num_iterators += 1;
+  
+  /* We need to use an index here when iterating _listeners->_items, not an
+  iterator. This is because a listener may add new listeners, causing the
+  vector to be reallocated, which would invalidate any iterator. */
+  for (size_t i = 0; i < _listeners->_items.size(); ++i) {
+    auto listener = _listeners->_items[i];
+    if (listener) {
+      try {
+        callback(listener);
+      }
+      catch (std::exception& e) {
+        SG_LOG(SG_GENERAL, SG_ALERT, "Ignoring exception from property callback: " << e.what());
+      }
+    }
+  }
+  
+  _listeners->_num_iterators -= 1;
+  
+  if (_listeners->_num_iterators == 0) {
+    /* Remove any items that have been set to nullptr. */
+    _listeners->_items.erase(
+        std::remove(_listeners->_items.begin(), _listeners->_items.end(), (SGPropertyChangeListener*) nullptr),
+        _listeners->_items.end()
+        );
+    if (_listeners->_items.empty()) {
+      delete _listeners;
+      _listeners = nullptr;
+    }
+  }
+}
+
+int SGPropertyNode::nListeners() const
+{
+  if (!_listeners) return 0;
+  int   n = 0;
+  for (auto listener: _listeners->_items) {
+    if (listener)   n += 1;
+  }
+  return n;
+}
+
 void
 SGPropertyNode::fireValueChanged (SGPropertyNode * node)
 {
-  if (_listeners != 0) {
+  forEachListener(
-    for (unsigned int i = 0; i < _listeners->size(); i++) {
+      node,
-        if ((*_listeners)[i])
+      _listeners,
-            (*_listeners)[i]->valueChanged(node);
+      [&](SGPropertyChangeListener* listener) { listener->valueChanged(node);}
-    }
+      );
-  }
   if (_parent != 0)
     _parent->fireValueChanged(node);
 }
@@ -2478,11 +2570,11 @@ void
 SGPropertyNode::fireChildAdded (SGPropertyNode * parent,
 				SGPropertyNode * child)
 {
-  if (_listeners != 0) {
+  forEachListener(
-    for (unsigned int i = 0; i < _listeners->size(); i++) {
+      parent,
-      (*_listeners)[i]->childAdded(parent, child);
+      _listeners,
-    }
+      [&](SGPropertyChangeListener* listener) { listener->childAdded(parent, child);}
-  }
+      );
   if (_parent != 0)
     _parent->fireChildAdded(parent, child);
 }
@@ -2491,11 +2583,11 @@ void
 SGPropertyNode::fireChildRemoved (SGPropertyNode * parent,
 				  SGPropertyNode * child)
 {
-  if (_listeners != 0) {
+  forEachListener(
-    for (unsigned int i = 0; i < _listeners->size(); i++) {
+      parent,
-      (*_listeners)[i]->childRemoved(parent, child);
+      _listeners,
-    }
+      [&](SGPropertyChangeListener* listener) { listener->childRemoved(parent, child);}
-  }
+      );
   if (_parent != 0)
     _parent->fireChildRemoved(parent, child);
 }

simgear/props/props.hxx

@@ -759,6 +759,9 @@ private:
 };
 
 
+struct SGPropertyNodeListeners;
+
+
 /**
  * A node in a property tree.
  */
@@ -1713,7 +1716,7 @@ public:
   /**
    * Get the number of listeners.
    */
-  int nListeners () const { return _listeners ? (int)_listeners->size() : 0; }
+  int nListeners () const;
 
 
   /**
@@ -1847,7 +1850,7 @@ private:
     char * string_val;
   } _local_val;
 
-  std::vector<SGPropertyChangeListener *> * _listeners;
+  SGPropertyNodeListeners*  _listeners;
 
   // Pass name as a pair of iterators
   template<typename Itr>

simgear/props/props_test.cxx

@@ -451,13 +451,17 @@ void defineSamplePropertyTree(SGPropertyNode_ptr root)
 class TestListener : public SGPropertyChangeListener
 {
 public:
-    TestListener(SGPropertyNode* root, bool recursive = false) :
+    TestListener(SGPropertyNode* root, bool recursive = false, bool self_unregister = false) :
-        _root(root) {}
+        _root(root), _self_unregister(self_unregister) {}
 
     virtual void valueChanged(SGPropertyNode* node) override
     {
         std::cout << "saw value changed for:" << node->getPath() << std::endl;
         valueChangeCount[node]++;
+        if (_self_unregister) {
+            std::cout << "valueChanged(): calling removeChangeListener() to self-remove\n";
+            node->removeChangeListener(this);
+        }
     }
 
     int checkValueChangeCount(const std::string& path) const
@@ -533,6 +537,7 @@ public:
         }
 private:
     SGPropertyNode* _root;
+    bool            _self_unregister;
     std::map<SGPropertyNode*, unsigned int> valueChangeCount;
 
     std::vector<ParentChange> adds;
@@ -942,6 +947,13 @@ void testDeleterListener()
         SG_VERIFY(ensureNListeners(tree, 0));
     }
 
+    // Self-unregister. prior to 2019-07-08 this would segv.
+    {
+        std::shared_ptr<TestListener>   l1( new TestListener(tree.get(), true /* recursive */, true /*self_unregister*/));
+        tree->setFloatValue("position/body/sub/self-unregister", 0.1);
+        tree->getNode("position/body/sub/self-unregister")->addChangeListener(l1.get());
+        tree->setFloatValue("position/body/sub/self-unregister", 0.2);
+    }
 }
 
 int main (int ac, char ** av)