From 1d978429f59ed8708129ad0f7b757ddf8dad4b8f Mon Sep 17 00:00:00 2001 From: Julian Smith Date: Sun, 12 Apr 2020 17:55:52 +0100 Subject: [PATCH] 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 | 146 ++++++++++++++++++++++++++++------- simgear/props/props.hxx | 7 +- simgear/props/props_test.cxx | 16 +++- 3 files changed, 138 insertions(+), 31 deletions(-) diff --git a/simgear/props/props.cxx b/simgear/props/props.cxx index cc3a1b19..b7bf5367 100644 --- a/simgear/props/props.cxx +++ b/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 _items; +}; + + //////////////////////////////////////////////////////////////////////// // Local classes. //////////////////////////////////////////////////////////////////////// @@ -968,7 +980,7 @@ SGPropertyNode::~SGPropertyNode () if (_listeners) { vector::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; - _listeners->push_back(listener); + _listeners = new SGPropertyNodeListeners; + + /* 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::iterator it = - find(_listeners->begin(), _listeners->end(), listener); - if (it != _listeners->end()) { - _listeners->erase(it); - listener->unregister_property(this); - if (_listeners->empty()) { - vector* tmp = _listeners; - _listeners = 0; - delete tmp; + find(_listeners->_items.begin(), _listeners->_items.end(), listener); + if (it != _listeners->_items.end()) { + if (_listeners->_num_iterators) { + /* _listeners._items is currently being iterated further up the stack in + this thread by one or more nested invocations of forEachListener(), so + we must be careful to not break these iterators. So we must not delete + _listeners. And we must not erase this entry from the vector because that + 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 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 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) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - if ((*_listeners)[i]) - (*_listeners)[i]->valueChanged(node); - } - } + forEachListener( + node, + _listeners, + [&](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) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - (*_listeners)[i]->childAdded(parent, child); - } - } + forEachListener( + parent, + _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) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - (*_listeners)[i]->childRemoved(parent, child); - } - } + forEachListener( + parent, + _listeners, + [&](SGPropertyChangeListener* listener) { listener->childRemoved(parent, child);} + ); if (_parent != 0) _parent->fireChildRemoved(parent, child); } diff --git a/simgear/props/props.hxx b/simgear/props/props.hxx index d1e8dd6a..828fe7c5 100644 --- a/simgear/props/props.hxx +++ b/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 * _listeners; + SGPropertyNodeListeners* _listeners; // Pass name as a pair of iterators template diff --git a/simgear/props/props_test.cxx b/simgear/props/props_test.cxx index 14211040..51fe0977 100644 --- a/simgear/props/props_test.cxx +++ b/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) : - _root(root) {} + TestListener(SGPropertyNode* root, bool recursive = false, bool self_unregister = false) : + _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 valueChangeCount; std::vector 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 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)