From: Simon Marchi Date: Wed, 11 Dec 2019 18:43:34 +0000 (-0500) Subject: lib: run most of bt_self_component_port_input_message_iterator_try_finalize when... X-Git-Url: http://git.efficios.com/?a=commitdiff_plain;h=fca28f7526e7f67c7314df3a113470e7bd109062;hp=fca28f7526e7f67c7314df3a113470e7bd109062;p=babeltrace.git lib: run most of bt_self_component_port_input_message_iterator_try_finalize when iterator is in NON_INITIALIZED state We hit a failed BT_ASSERT in the specific scenario explained below (and illustrated by the new test). The problem ----------- Let's say we have a graph with this topology, where everything is implemented in Python: MySink [in] <-> [out] MyFilter (MyFilterIter) [in] <-> [out] MySource (MySourceIter) And the following sequence of events: 1. MySink's _user_graph_is_configured method creates an iterator on its "in" input port. This runs MyFilterIter's __init__ method. 2. MyFilterIter's __init__ method starts by creating an upstream iterator on its "in" input port. This iterator, of type MySourceIter, is initialized successfully. 3. MyFilterIter's __init__ method creates some other data structure which happens to form a Python reference cycle (the MyFilterIter instance has a reference on an object, which has a reference on the MyFilterIter instance). 4. MyFilterIter's __init__ method encounters an error, so an exception is raised. When the MySourceIter is created, an entry is added to MyFilterIter's upstream_msg_iters array (in the underlying bt_self_component_port_input_message_iterator object). When the exception is raised, because of the reference cycle, the MyFilterIter Python object stays alive. It has a reference on the MySourceIter Python object, which keeps the underlying bt_self_component_port_input_message_iterator object alive as well. When the create_self_component_input_port_message_iterator call realizes that the initialization of the filter iterator failed, it does a put_ref on the iterator object, which ends up calling bt_self_component_port_input_message_iterator_destroy. In there, we assert that: BT_ASSERT(iterator->upstream_msg_iters->len == 0); This assertion does not hold, because the array still contains the entry for the upstream iterator (on MySource). Normally, the call to bt_self_component_port_input_message_iterator_try_finalize, earlier in bt_self_component_port_input_message_iterator_destroy, should take care of unlinking any upstream or downstream iterator. However, that step is completely skipped because the iterator is still in the NON_INITIALIZED state. To reproduce, it is important to have the two conditions before the exception is raised: - an upstream iterator is created: otherwise, the upstream_msg_iters array is empty so the assertion is true - a Python reference cycle exists, keeping the MyFilterIter object alive: otherwise, it is destroyed when the exception is raised (or caught?), which destroys the MySourceIter object, which destroys the underlying bt_self_component_port_input_message_iterator object (or the source iterator), which removes itself from the filter iterator's upstream_msg_iters array. The array now being empty, the assertion would be true. Note that the Python reference cycle is a user error that is not desirable, but likely to happen. To avoid any memory leak due to such a reference cycle, I think the filter iterator here should normally make sure to break the reference cycle. In a successful execution, it would be in _user_finalize. If __init__ fails, it should make sure to not leave a reference cycle in place, perhaps by using a try-except to clear it before exiting the function. The fix ------- I believe it's wrong to skip the _try_finalize function even when we are in this state, because it's possible (as shown above) for an iterator initialize method to create some upstream iterators but then fail. The iterator remains in the NON_INITIALIZED state, but has some upstream iterators. I have changed bt_self_component_port_input_message_iterator_try_finalize so that when the iterator is in the NON_INITIALIZED, we do unlink the upstream and downstream iterators. However, I don't think we want to call the iterator's finalize method in that case, since it hasn't been properly initialized (a bit like a C++ object destructor is not called if the constructor throws). So I made it so the finalize method is only called if the state is not NON_INITIALIZED. Change-Id: Ibe2cbc729fc81ff0d68219400d925110242b7ae1 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/2633 Tested-by: jenkins Reviewed-by: Francis Deslauriers ---