From ec656f034d59b40463c72d8176fd8277436890ab Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 1 May 2019 18:00:50 -0400 Subject: [PATCH] bt2: Add wrapper for bt_plugin_get_version There is a subtle bug with our use of the "char **OUT" typemap for the bt_plugin_get_version function. THe SWIG wrapper does this: char *temp_value5; arg5 = &temp_value5; The initial content of temp_value5 is unknown, assume that it points to unreadable memory. If the call to bt_plugin_get_version returns PROPERTY_AVAILABILITY_NOT_AVAILABLE, the value of temp_value5 is unchanged (but really, it could be anything, again we should assume it points to unreadable memory). However, the epilogue of the typemap will still try to transform the C string to a Python str object. Doing so will cause a crash when trying to read the C string. The fix is to make our own wrapper to bt_plugin_get_version, which guarantees that if it returns PROPERTY_AVAILABILITY_NOT_AVAILABLE, it will force the value of the output parameter to be NULL (which is handled by the epilogue and transformed to None). Setting the temporary variable to an invalid but non-zero pointer value ((void *) 1) makes this bug consistently reproducible and not rely on random memory contents. I suggest we keep it there, in order to quickly find other instances of this bug in the future. Change-Id: Idd12ca4c4d4d732b841c1e12420e46e6a5a25874 Reviewed-on: https://review.lttng.org/c/babeltrace/+/1221 Reviewed-by: Philippe Proulx --- bindings/python/bt2/bt2/native_bt.i | 12 ++++++-- bindings/python/bt2/bt2/native_bt_plugin.i | 33 ++++++++++++++++++++++ bindings/python/bt2/bt2/plugin.py | 2 +- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/bindings/python/bt2/bt2/native_bt.i b/bindings/python/bt2/bt2/native_bt.i index b1e2d27d..fe289b0b 100644 --- a/bindings/python/bt2/bt2/native_bt.i +++ b/bindings/python/bt2/bt2/native_bt.i @@ -52,8 +52,16 @@ typedef int bt_bool; %rename("%(strip:[bt_])s", %$isvariable) ""; %rename("%(strip:[BT_])s", %$isenumitem) ""; -/* Output argument typemap for string output (always appends) */ -%typemap(in, numinputs=0) (const char **OUT) (char *temp_value) { +/* + * Output argument typemap for string output (always appends) + * + * We initialize the output parameter `temp_value` to an invalid but non-zero + * pointer value. This is to make sure we don't rely on its initial value in + * the epilogue (where we call SWIG_Python_str_FromChar). When they fail, + * functions on which we apply this typemap don't guarantee that the value of + * `temp_value` will be unchanged or valid. + */ +%typemap(in, numinputs=0) (const char **OUT) (char *temp_value = (void *) 1) { $1 = &temp_value; } diff --git a/bindings/python/bt2/bt2/native_bt_plugin.i b/bindings/python/bt2/bt2/native_bt_plugin.i index 640085be..435d40f2 100644 --- a/bindings/python/bt2/bt2/native_bt_plugin.i +++ b/bindings/python/bt2/bt2/native_bt_plugin.i @@ -96,3 +96,36 @@ extern const bt_plugin *bt_plugin_set_borrow_plugin_by_index_const( extern void bt_plugin_set_get_ref(const bt_plugin_set *plugin_set); extern void bt_plugin_set_put_ref(const bt_plugin_set *plugin_set); + +/* Helpers */ + +bt_property_availability bt_plugin_get_version_wrapper( + const bt_plugin *plugin, unsigned int *OUT, + unsigned int *OUT, unsigned int *OUT, const char **OUT); + +%{ + +/* + * This wrapper ensures that when the API function fails, the `*extra` output + * parameter is set to NULL. This is necessary because the epilogue of the + * "char **OUT" typemap will use that value to make a Python str object. We + * can't rely on the initial value of `*extra`, it could point to unreadable + * memory. + */ + +bt_property_availability bt_plugin_get_version_wrapper( + const bt_plugin *plugin, unsigned int *major, + unsigned int *minor, unsigned int *patch, const char **extra) +{ + bt_property_availability ret; + + ret = bt_plugin_get_version(plugin, major, minor, patch, extra); + + if (ret == BT_PROPERTY_AVAILABILITY_NOT_AVAILABLE) { + *extra = NULL; + } + + return ret; +} + +%} diff --git a/bindings/python/bt2/bt2/plugin.py b/bindings/python/bt2/bt2/plugin.py index 51f9a2ed..39223a8c 100644 --- a/bindings/python/bt2/bt2/plugin.py +++ b/bindings/python/bt2/bt2/plugin.py @@ -198,7 +198,7 @@ class _Plugin(object._SharedObject): @property def version(self): - status, major, minor, patch, extra = native_bt.plugin_get_version(self._ptr) + status, major, minor, patch, extra = native_bt.plugin_get_version_wrapper(self._ptr) if status == native_bt.PROPERTY_AVAILABILITY_NOT_AVAILABLE: return -- 2.34.1