[CSA Bug Report]Clang Static Analyzer(CSA) Memory Bug Detected
Nobody has claimed this yet.
- Dominant language
- EmberScript
- Stars
- 26
- Forks
- 68
- Avg merge
- 2d 13h
- Merged PRs (30d)
- 1
Description
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.
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: 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.
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
Fix: I think it's better to add Py_DECREF(_pymessage) before return NULL;
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with rosidl_generator_py/resource/_msg_support.c.em, especially lines 179-193 and the error-return paths from lines 538 onward. Trace the ownership of name_attr, module_attr, class_attr, and _pymessage through each failure path. Done means borrowed string data is not used after its owner is released and allocated _pymessage objects are released before every NULL or false return.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, python
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100