From 3443d1cdd32b3d09deb4d2d08e18bbf63b449416 Mon Sep 17 00:00:00 2001 From: Martins Mozeiko Date: Thu, 5 Jul 2018 16:36:49 -0700 Subject: [PATCH 1/2] fix memory leaks when converting Python types to native types This separates memory allocation out from convert_from_py function. Now it uses separate create_message function to allocate message, making it explicit gives better control where and how memory is allocated and freed. --- rosidl_generator_py/resource/_msg.py.em | 2 + .../_msg_pkg_typesupport_entry_point.c.em | 6 +- .../resource/_msg_support.c.em | 58 ++++++++++--------- 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/rosidl_generator_py/resource/_msg.py.em b/rosidl_generator_py/resource/_msg.py.em index 73708039..8c1ff0a2 100644 --- a/rosidl_generator_py/resource/_msg.py.em +++ b/rosidl_generator_py/resource/_msg.py.em @@ -21,6 +21,7 @@ import traceback class Metaclass(type): """Metaclass of message '@(spec.base_type.type)'.""" + _CREATE_ROS_MESSAGE = None _CONVERT_FROM_PY = None _CONVERT_TO_PY = None _DESTROY_ROS_MESSAGE = None @@ -42,6 +43,7 @@ class Metaclass(type): logger.debug( 'Failed to import needed modules for type support:\n' + traceback.format_exc()) else: + cls._CREATE_ROS_MESSAGE = module.create_ros_message_msg_@(module_name) cls._CONVERT_FROM_PY = module.convert_from_py_msg_@(module_name) cls._CONVERT_TO_PY = module.convert_to_py_msg_@(module_name) cls._TYPE_SUPPORT = module.type_support_msg_@(module_name) diff --git a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em index 1410ecdc..ad397221 100644 --- a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em +++ b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em @@ -16,6 +16,7 @@ @ #include #include +#include @{ static_includes = set([ @@ -55,8 +56,9 @@ for spec, subfolder in service_specs: type_name = spec.base_type.type module_name = convert_camel_case_to_lower_case_underscore(type_name) }@ -void * @(spec.base_type.pkg_name)_@(module_name)__convert_from_py(PyObject * _pymsg); +void * @(spec.base_type.pkg_name)_@(module_name)__create_ros_message(void); void @(spec.base_type.pkg_name)_@(module_name)__destroy_ros_message(void * raw_ros_message); +bool @(spec.base_type.pkg_name)_@(module_name)__convert_from_py(PyObject * _pymsg, void * ros_message); PyObject * @(spec.base_type.pkg_name)_@(module_name)__convert_to_py(void * raw_ros_message); @[end for]@ @@ -79,7 +81,7 @@ static struct PyModuleDef @(package_name)__module = { @[for spec, subfolder in message_specs]@ @{ type_name = convert_camel_case_to_lower_case_underscore(spec.base_type.type) -function_names = ['convert_from_py', 'destroy_ros_message', 'convert_to_py', 'type_support'] +function_names = ['create_ros_message', 'destroy_ros_message', 'convert_from_py', 'convert_to_py', 'type_support'] }@ ROSIDL_GENERATOR_C_IMPORT diff --git a/rosidl_generator_py/resource/_msg_support.c.em b/rosidl_generator_py/resource/_msg_support.c.em index 93605fc9..cecf816c 100644 --- a/rosidl_generator_py/resource/_msg_support.c.em +++ b/rosidl_generator_py/resource/_msg_support.c.em @@ -13,6 +13,7 @@ @####################################################################### @ #include +#include #include <@(spec.base_type.pkg_name)/@(subfolder)/@(module_name)__struct.h> #include <@(spec.base_type.pkg_name)/@(subfolder)/@(module_name)__functions.h> @@ -52,7 +53,7 @@ if not field.type.is_primitive_type() and field.type.is_array: msg_typename = '%s__%s__%s' % (spec.base_type.pkg_name, subfolder, spec.base_type.type) }@ -void * @(spec.base_type.pkg_name)_@(module_name)__convert_from_py(PyObject * _pymsg) +bool @(spec.base_type.pkg_name)_@(module_name)__convert_from_py(PyObject * _pymsg, void * _ros_message) { @{ full_classname = '%s.%s._%s.%s' % (spec.base_type.pkg_name, subfolder, module_name, spec.base_type.type) @@ -80,7 +81,7 @@ full_classname = '%s.%s._%s.%s' % (spec.base_type.pkg_name, subfolder, module_na } } if (!class_name || !module_name) { - return NULL; + return false; } snprintf(full_classname_dest, sizeof(full_classname_dest), "%s.%s", module_name, class_name); } @@ -88,7 +89,7 @@ full_classname = '%s.%s._%s.%s' % (spec.base_type.pkg_name, subfolder, module_na "@(full_classname)", full_classname_dest, @(len(full_classname))) == 0); } - @(msg_typename) * ros_message = @(msg_typename)__create(); + @(msg_typename) * ros_message = _ros_message; @[if not spec.fields]@ (void)ros_message; @[end if]@ @@ -96,64 +97,64 @@ full_classname = '%s.%s._%s.%s' % (spec.base_type.pkg_name, subfolder, module_na { // @(field.name) PyObject * field = PyObject_GetAttrString(_pymsg, "@(field.name)"); if (!field) { - return NULL; + return false; } @[ if not field.type.is_primitive_type()]@ @{ nested_type = '%s__%s__%s' % (field.type.pkg_name, 'msg', field.type.type) }@ - typedef PyObject *(* convert_from_py_signature)(void *); + typedef bool (* convert_from_py_signature)(PyObject *, void *); convert_from_py_signature convert_from_py = NULL; // get conversion function { PyObject * msg_module = PyImport_ImportModule("@(field.type.pkg_name).msg._@convert_camel_case_to_lower_case_underscore(field.type.type)"); if (!msg_module) { Py_DECREF(field); - return NULL; + return false; } PyObject * msg_class = PyObject_GetAttrString(msg_module, "@(field.type.type)"); Py_DECREF(msg_module); if (!msg_class) { Py_DECREF(field); - return NULL; + return false; } PyObject * msg_metaclass = PyObject_GetAttrString(msg_class, "__class__"); Py_DECREF(msg_class); if (!msg_metaclass) { Py_DECREF(field); - return NULL; + return false; } PyObject * convert_from_py_ = PyObject_GetAttrString(msg_metaclass, "_CONVERT_FROM_PY"); Py_DECREF(msg_metaclass); if (!convert_from_py_) { Py_DECREF(field); - return NULL; + return false; } convert_from_py = (convert_from_py_signature)PyCapsule_GetPointer(convert_from_py_, NULL); Py_DECREF(convert_from_py_); if (!convert_from_py) { Py_DECREF(field); - return NULL; + return false; } } @[ if field.type.is_array]@ PyObject * seq_field = PySequence_Fast(field, "expected a sequence in '@(field.name)'"); if (!seq_field) { Py_DECREF(field); - return NULL; + return false; } @[ if field.type.array_size is None or field.type.is_upper_bound]@ Py_ssize_t size = PySequence_Size(field); if (-1 == size) { Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } if (!@(nested_type)__Array__init(&(ros_message->@(field.name)), size)) { PyErr_SetString(PyExc_RuntimeError, "unable to create @(nested_type)__Array ros_message"); Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } @(nested_type) * dest = ros_message->@(field.name).data; @[ else]@ @@ -161,49 +162,45 @@ nested_type = '%s__%s__%s' % (field.type.pkg_name, 'msg', field.type.type) @(nested_type) * dest = ros_message->@(field.name); @[ end if]@ for (Py_ssize_t i = 0; i < size; ++i) { - @(nested_type) * item = (@(nested_type) *) convert_from_py( - PySequence_Fast_GET_ITEM(seq_field, i)); - if (!item) { + if (!convert_from_py(PySequence_Fast_GET_ITEM(seq_field, i), &dest[i])) { Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } - memcpy(&dest[i], item, sizeof(@(nested_type))); } Py_DECREF(seq_field); @[ else]@ - @(nested_type) * tmp = (@(nested_type) *) convert_from_py(field); - if (!tmp) { - return NULL; + if (!convert_from_py(field, &ros_message->@(field.name))) { + Py_DECREF(field); + return false; } - ros_message->@(field.name) = *tmp; @[ end if]@ @[ elif field.type.is_array]@ PyObject * seq_field = PySequence_Fast(field, "expected a sequence in '@(field.name)'"); if (!seq_field) { Py_DECREF(field); - return NULL; + return false; } @[ if field.type.array_size is None or field.type.is_upper_bound]@ Py_ssize_t size = PySequence_Size(field); if (-1 == size) { Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } @[ if field.type.type == 'string']@ if (!rosidl_generator_c__String__Array__init(&(ros_message->@(field.name)), size)) { PyErr_SetString(PyExc_RuntimeError, "unable to create String__Array ros_message"); Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } @[ else]@ if (!rosidl_generator_c__@(field.type.type)__Array__init(&(ros_message->@(field.name)), size)) { PyErr_SetString(PyExc_RuntimeError, "unable to create @(field.type.type)__Array ros_message"); Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } @[ end if]@ @primitive_msg_type_to_c(field.type.type) * dest = ros_message->@(field.name).data; @@ -216,7 +213,7 @@ nested_type = '%s__%s__%s' % (field.type.pkg_name, 'msg', field.type.type) if (!item) { Py_DECREF(seq_field); Py_DECREF(field); - return NULL; + return false; } @[ if field.type.type == 'char']@ assert(PyUnicode_Check(item)); @@ -317,7 +314,12 @@ nested_type = '%s__%s__%s' % (field.type.pkg_name, 'msg', field.type.type) } @[end for]@ - return ros_message; + return true; +} + +void * @(spec.base_type.pkg_name)_@(module_name)__create_ros_message(void) +{ + return @(msg_typename)__create(); } void @(spec.base_type.pkg_name)_@(module_name)__destroy_ros_message(void * raw_ros_message) From bc8d40ed4e1806c4410455f370ef88eb3bf852f1 Mon Sep 17 00:00:00 2001 From: Martins Mozeiko Date: Wed, 11 Jul 2018 14:14:16 -0700 Subject: [PATCH 2/2] Rearranged order of includes. --- .../resource/_msg_pkg_typesupport_entry_point.c.em | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em index ad397221..a4820d6f 100644 --- a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em +++ b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em @@ -15,8 +15,8 @@ @####################################################################### @ #include -#include #include +#include @{ static_includes = set([