rosidl_python icon indicating copy to clipboard operation
rosidl_python copied to clipboard
verified publisher icon ros2

[CSA Bug Report]Clang Static Analyzer(CSA) Memory Bug Detected

Open wr-web opened this issue 1 year ago • 2 comments

Bug report

Clang Static Analyzer(CSA), pyrefcon @Snape3058 (http://lcs.ios.ac.cn/~maxt/PyRefcon/ASE-2023.pdf)

  • Operating System:
    • Linux d18de72e1bb7 5.4.0-196-generic x86

Bug Type: Access released Memory/Use After Free

File: _msg_support.c.em Commit: https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L179-L193 Detail: after Py_DECREF module_attr and class_attr may be released, module_name and class_name are possible freed, Causing Access released Memory/Use After Free.

Prove of Content(POC):

static *
poc(PyObject * object) {
  PyObject * module_attr = PyObject_GetAttrString(object, "__class__");
  char * module_name = NULL;
  if (module_attr) {
    PyObject *name_attr = PyObject_GetAttrString(module_attr, "__name__");
    if (name_attr) {
      module_name = (char *)PyUnicode_1BYTE_DATA(name_attr);
      Py_DECREF(name_attr);
      Py_DECREF(module_attr);
      printf("%s", module_name);
    }
  }
  return PyLong_FromLong(0);
}

And this is the correct result. image

However, If the garbege collect was triggered between Py_DECREF(object) and usage of string module_name, things will become troublesome.

static *
poc(PyObject * object) {
  PyObject * module_attr = PyObject_GetAttrString(object, "__class__");
  char * module_name = NULL;
  if (module_attr) {
    PyObject *name_attr = PyObject_GetAttrString(module_attr, "__name__");
    if (name_attr) {
      module_name = (char *)PyUnicode_1BYTE_DATA(name_attr);
      Py_DECREF(name_attr);
      Py_DECREF(module_attr);
      call_gc_collect();
      printf("%s\n", module_name);
    }
  }
  return PyLong_FromLong(0);
}

void call_gc_collect() {
    PyObject *gc_module = PyImport_ImportModule("gc");
    if (gc_module) {
        PyObject *gc_collect = PyObject_GetAttrString(gc_module, "collect");
        if (gc_collect && PyCallable_Check(gc_collect)) {
            PyObject *result = PyObject_CallObject(gc_collect, NULL);
            Py_XDECREF(result);
        }
        Py_XDECREF(gc_collect);
        Py_DECREF(gc_module);
    }
}

This is the result: image

Finding that module_name has been freed. In this case, I manually call gc.collect() to explain it. In real python environment, GC could free module_name at any time, Causing Use After Free Bug.

How to Fix: I think it's better to use these string before Py_DECREF:

      {
        PyObject * class_attr = PyObject_GetAttrString(_pymsg, "__class__");
        if (class_attr) {
          PyObject * name_attr = PyObject_GetAttrString(class_attr, "__name__");
          if (name_attr) {
            class_name = (char *)PyUnicode_1BYTE_DATA(name_attr);
          }
          PyObject * module_attr = PyObject_GetAttrString(class_attr, "__module__");
          if (module_attr) {
            module_name = (char *)PyUnicode_1BYTE_DATA(module_attr);
          }
          if (!class_name || !module_name) {
            return false;
          }
          snprintf(full_classname_dest, sizeof(full_classname_dest), "%s.%s", module_name, class_name);
          Py_XDECREF(module_attr);
          Py_XDECREF(name_attr);
          Py_DECREF(class_attr);
        }
      }

Bug Type: Non-Zero Dead Object/Memory Leak

File: _msg_support.c.em Commit: https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L538 Detail: If _pymessage has been correctly allocated, function may return NULL without freeing _pymessage, Causing Non-Zero Dead Object/Memory Leak.

Code: https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L560-L563

    field = PyObject_GetAttrString(_pymessage, "@(member.name)");
    if (!field) {
      return NULL;
    }

Detail: if PyObject_GetAttrString fail and return NULL, function will return NULL causing _pymessage leak.

Same in any code block fail and return NULL or false: https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L576-L579

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L584-L592

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L594-L598

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L603-L608

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L620-L626

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L642-L645

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L653-L657

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L664-L667

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L677-L680

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L691-L694

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L700-L703

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L743-L747

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L748-L752

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L754-L760

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L763-L769

https://github.com/ros2/rosidl_python/blob/1fbd99b5fe1fa04674c73e7e5992ccc88e28157c/rosidl_generator_py/resource/_msg_support.c.em#L795-L799

Fix: I think it's better to add Py_DECREF(_pymessage) before return NULL;

wr-web avatar Sep 27 '24 10:09 wr-web

Thanks for the report.

Bug Type: Access released Memory/Use After Free ... How to Fix: I think it's better to use these string before Py_DECREF:

Yes, completely agreed, that is better.

Bug Type: Non-Zero Dead Object/Memory Leak ... Fix: I think it's better to add Py_DECREF(_pymessage) before return NULL;

That won't completely fix the issue. It will fix the issue with _pymessage itself, but it won't fix the fact that we may allocate some fields, but then fail later on. In which case, we have to drop the work we've already done. The error handling in this function needs to be significantly rethought.

clalancette avatar Oct 02 '24 12:10 clalancette

See https://github.com/ros2/rosidl_python/pull/218, which at least fixes the first bug reported here.

clalancette avatar Oct 02 '24 14:10 clalancette