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

[wr-web]

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:

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 179 to 193 in 1fbd99b

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

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.

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:

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:

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Line 538 in 1fbd99b

if (!_pymessage) {

Detail: If _pymessage has been correctly allocated, function may return NULL without freeing _pymessage, Causing Non-Zero Dead Object/Memory Leak.

Code:

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 560 to 563 in 1fbd99b

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

    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:

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 576 to 579 in 1fbd99b

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

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 584 to 592 in 1fbd99b

	PyObject * itemsize_attr = PyObject_GetAttrString(field, "itemsize");
	assert(itemsize_attr != NULL);
	size_t itemsize = PyLong_AsSize_t(itemsize_attr);
	Py_DECREF(itemsize_attr);
	if (itemsize != sizeof(@primitive_msg_type_to_c(member.type.value_type))) {
	PyErr_SetString(PyExc_RuntimeError, "itemsize doesn't match expectation");
	Py_DECREF(field);
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 594 to 598 in 1fbd99b

	Py_ssize_t length = PyObject_Length(field);
	if (-1 == length) {
	Py_DECREF(field);
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 603 to 608 in 1fbd99b

	PyObject * ret = PyObject_CallFunctionObjArgs(pop, NULL);
	if (!ret) {
	Py_DECREF(pop);
	Py_DECREF(field);
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 620 to 626 in 1fbd99b

	PyObject * ret = PyObject_CallFunctionObjArgs(frombytes, data, NULL);
	Py_DECREF(data);
	Py_DECREF(frombytes);
	if (!ret) {
	Py_DECREF(field);
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 642 to 645 in 1fbd99b

	field = PyList_New(size);
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 653 to 657 in 1fbd99b

	PyObject * pyitem = @('__'.join(type_.namespaces + [convert_camel_case_to_lower_case_underscore(type_.name)]))__convert_to_py(item);
	if (!pyitem) {
	Py_DECREF(field);
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 664 to 667 in 1fbd99b

	field = @('__'.join(type_.namespaces + [convert_camel_case_to_lower_case_underscore(type_.name)]))__convert_to_py(&ros_message->@(member.name));
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 677 to 680 in 1fbd99b

	field = PyList_New(size);
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 691 to 694 in 1fbd99b

	PyObject * decoded_item = PyUnicode_DecodeUTF8(src[i].data, strlen(src[i].data), "replace");
	if (!decoded_item) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 700 to 703 in 1fbd99b

	PyObject * decoded_item = PyUnicode_DecodeUTF16((const char *)src[i].data, src[i].size * sizeof(uint16_t), NULL, &byteorder);
	if (!decoded_item) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 743 to 747 in 1fbd99b

	@[ elif isinstance(member.type, BasicType) and member.type.typename == 'char']@
	field = Py_BuildValue("C", ros_message->@(member.name));
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 748 to 752 in 1fbd99b

	@[ elif isinstance(member.type, BasicType) and member.type.typename == 'octet']@
	field = PyBytes_FromStringAndSize((const char *)&ros_message->@(member.name), 1);
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 754 to 760 in 1fbd99b

	field = PyUnicode_DecodeUTF8(
	ros_message->@(member.name).data,
	strlen(ros_message->@(member.name).data),
	"replace");
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 763 to 769 in 1fbd99b

	field = PyUnicode_DecodeUTF16(
	(const char *)ros_message->@(member.name).data,
	ros_message->@(member.name).size * sizeof(uint16_t),
	NULL, &byteorder);
	if (!field) {
	return NULL;
	}

rosidl_python/rosidl_generator_py/resource/_msg_support.c.em

Lines 795 to 799 in 1fbd99b

	int rc = PyObject_SetAttrString(_pymessage, "@(member.name)", field);
	Py_DECREF(field);
	if (rc) {
	return NULL;
	}

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

[clalancette]

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]

See #218, which at least fixes the first bug reported here.