From b5947615bfcd6a309e1945a270f67ccc6cd2cf69 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 9 Jul 2019 17:11:33 -0400 Subject: [PATCH] Fix: bt2: adjust reference counting of value objects This patch fixes some reference counting bugs when handling bt_values in Python. The first one is when a component class implemented in Python returns None to a query. The _UserComponentType._bt_query_from_native method must return a bt_value address, and it must be a new reference that it transfers to its caller. Currently, it returns a pointer to bt_value_null, without ever acquiring a new reference to it. The second bug, in the same area, is when the component class returns some value. We currently call bt2.object._release, effectively transferring the Python object's reference to the value to the caller. However, the user could have kept a reference to that value, wishing to keep using it. We should instead return a new reference to this value. _UserComponentType._bt_query_from_native is therefore modified to acquire a new reference to return to its caller, regardless of if the value is bt_value_null or another value. The third bug is in bt2.value._create_from_ptr. This function receives a pointer to a bt_value, and steals it from its caller. It creates a Python object that, when destroy, will put the reference. However, when the passed pointer is bt_value_null, it just returns None. The reference to bt_value_null that has been passed therefore never gets put. Bugs 1 and 3 would cancel each other: when doing a query that returned bt_value_null from Python to a Python component class, we would miss getting a ref to bt_value_null (bug 1) but we would also miss putting it (bug 3). The patch adds a test where Python code queries a Python component class, which returns None. While it might not have caught all the bugs fixed in this patch, it at least exercises the code paths touched in this patch, which were not tested before, AFAICT. Change-Id: I0f7bf1004d9d766db4cc0c83475ed37ce2e3f1ed Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/1666 CI-Build: Philippe Proulx Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/bindings/python/bt2/bt2/component.py | 9 ++++++--- src/bindings/python/bt2/bt2/object.py | 7 ------- src/bindings/python/bt2/bt2/value.py | 9 ++++++++- tests/bindings/python/bt2/test_component_class.py | 12 ++++++++++++ 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/bindings/python/bt2/bt2/component.py b/src/bindings/python/bt2/bt2/component.py index afd7514a..9834fa8c 100644 --- a/src/bindings/python/bt2/bt2/component.py +++ b/src/bindings/python/bt2/bt2/component.py @@ -527,12 +527,15 @@ class _UserComponentType(type): results = bt2.create_value(results) if results is None: - results_addr = int(native_bt.value_null) + results_ptr = native_bt.value_null else: # return new reference - results_addr = int(results._release()) + results_ptr = results._ptr - return results_addr + # We return a new reference. + bt2.value._Value._get_ref(results_ptr) + + return int(results_ptr) def _query(cls, query_executor, obj, params, log_level): raise NotImplementedError diff --git a/src/bindings/python/bt2/bt2/object.py b/src/bindings/python/bt2/bt2/object.py index 218965de..f73ec04b 100644 --- a/src/bindings/python/bt2/bt2/object.py +++ b/src/bindings/python/bt2/bt2/object.py @@ -127,12 +127,5 @@ class _SharedObject(_BaseObject): cls._get_ref(obj._ptr) return obj - def _release(self): - """Return the wrapped pointer, transfer its ownership to the - caller.""" - ptr = self._ptr - self._ptr = None - return ptr - def __del__(self): self._put_ref(self._ptr) diff --git a/src/bindings/python/bt2/bt2/value.py b/src/bindings/python/bt2/bt2/value.py index b6a5a4a1..2710d903 100644 --- a/src/bindings/python/bt2/bt2/value.py +++ b/src/bindings/python/bt2/bt2/value.py @@ -30,7 +30,14 @@ import bt2 def _create_from_ptr(ptr): - if ptr is None or ptr == native_bt.value_null: + if ptr is None: + return + + # bt_value_null is translated to None. However, we are given a reference + # to it that we are not going to manage anymore, since we don't create a + # Python wrapper for it. Therefore put that reference immediately. + if ptr == native_bt.value_null: + bt2.value._Value._put_ref(ptr) return typeid = native_bt.value_get_type(ptr) diff --git a/tests/bindings/python/bt2/test_component_class.py b/tests/bindings/python/bt2/test_component_class.py index 73f0439b..bc60606c 100644 --- a/tests/bindings/python/bt2/test_component_class.py +++ b/tests/bindings/python/bt2/test_component_class.py @@ -245,6 +245,18 @@ class UserComponentClassTestCase(unittest.TestCase): self.assertEqual(query_log_level, bt2.LoggingLevel.WARNING) del query_log_level + def test_query_returns_none(self): + class MySink(bt2._UserSinkComponent): + def _consume(self): + pass + + @staticmethod + def _query(query_exec, obj, params, log_level): + return + + res = bt2.QueryExecutor().query(MySink, 'obj', None) + self.assertIsNone(res) + def test_query_simple(self): class MySink(bt2._UserSinkComponent): def _consume(self): -- 2.34.1