After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 786872 - _PyGIDefaultArgPlaceholder is uninitialized, leading to memory corruption after GC
_PyGIDefaultArgPlaceholder is uninitialized, leading to memory corruption aft...
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-27 20:05 UTC by Daniel Colascione
Modified: 2017-10-26 07:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix potential uninitialized memory access during GC (1.54 KB, patch)
2017-10-24 12:47 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Daniel Colascione 2017-08-27 20:05:20 UTC
We use _PyGIDefaultArgPlaceholder as a sentinel value to represent default values during function argument list construction. Right now, it's a Python type object. We make it using PyObject_New, so most of its fields end up uninitialized. The object body being uninitialized wouldn't be a problem if the placeholder object were unreachable, but the object *can* be reached during GC by traversal through frame objects. From valgrind:

==1184== Conditional jump or move depends on uninitialised value(s)
==1184==    at 0x528801: visit_decref (gcmodule.c:373)
==1184==    by 0x472165: tupletraverse (tupleobject.c:564)
==1184==    by 0x528A84: subtract_refs (gcmodule.c:398)
==1184==    by 0x528A84: collect (gcmodule.c:969)
==1184==    by 0x529708: collect_with_callback (gcmodule.c:1140)
==1184==    by 0x529708: collect_generations (gcmodule.c:1163)
==1184==    by 0x529E30: _PyObject_GC_Malloc (gcmodule.c:1738)
==1184==    by 0x529E30: _PyObject_GC_NewVar (gcmodule.c:1765)
==1184==    by 0x5EBAD9: PyFrame_New (frameobject.c:671)
==1184==    by 0x4DD633: PyEval_EvalCodeEx (ceval.c:3414)
==1184==    by 0x5EC76B: function_call (funcobject.c:632)
==1184==    by 0x428FF9: PyObject_Call (abstract.c:2040)
==1184==    by 0x5E115C: method_call (classobject.c:347)
==1184==    by 0x428FF9: PyObject_Call (abstract.c:2040)
==1184==    by 0x47B2BA: slot_tp_init (typeobject.c:6177)
==1184==  Uninitialised value was created by a heap allocation
==1184==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1184==    by 0x464E5C: _PyObject_New (object.c:247)
==1184==    by 0xA6174F6: pyglib__gi_module_create (in /usr/lib/python3/dist-packages/gi/_gi.cpython-34m-x86_64-linux-gnu.so)
==1184==    by 0x4FAF66: _PyImport_LoadDynamicModule (importdl.c:88)
==1184==    by 0x4F709C: _imp_load_dynamic_impl (import.c:2263)
==1184==    by 0x4F709C: _imp_load_dynamic (import.c:2239)
==1184==    by 0x4DAD2A: ext_do_call (ceval.c:4558)
==1184==    by 0x4DAD2A: PyEval_EvalFrameEx (ceval.c:2878)
==1184==    by 0x4DDD03: PyEval_EvalCodeEx (ceval.c:3588)
==1184==    by 0x4DAA7F: fast_function (ceval.c:4344)
==1184==    by 0x4DAA7F: call_function (ceval.c:4262)
==1184==    by 0x4DAA7F: PyEval_EvalFrameEx (ceval.c:2838)
==1184==    by 0x4DDD03: PyEval_EvalCodeEx (ceval.c:3588)
==1184==    by 0x5EC856: function_call (funcobject.c:632)
==1184==    by 0x428FF9: PyObject_Call (abstract.c:2040)
==1184==    by 0x4D67D6: ext_do_call (ceval.c:4561)
==1184==    by 0x4D67D6: PyEval_EvalFrameEx (ceval.c:2878)

Depending on the exact contents of the uninitialized memory, the GC can go on to cause other kinds of memory corruption through the process.

IMHO, the easiest fix for this problem is to just make the placeholder a simpler data structure, like a list. PyType_Type is also a pig, memory-wise: its tp_basicsize is 824 bytes here!

diff --git a/gi/gimodule.c b/gi/gimodule.c
index e14b4f6a..5f8853c8 100644
--- a/gi/gimodule.c
+++ b/gi/gimodule.c
@@ -730,7 +730,7 @@ PYGLIB_MODULE_START(_gi, "_gi")
     /* Place holder object used to fill in "from Python" argument lists
      * for values not supplied by the caller but support a GI default.
      */
-    _PyGIDefaultArgPlaceholder = PyObject_New(PyObject, &PyType_Type);
+    _PyGIDefaultArgPlaceholder = PyList_New(0);
 
     Py_INCREF (PyGIWarning);
     PyModule_AddObject (module, "PyGIWarning", PyGIWarning);
Comment 1 Christoph Reiter (lazka) 2017-10-24 12:47:34 UTC
Created attachment 362178 [details] [review]
Fix potential uninitialized memory access during GC

Sounds good. I went ahead and turned it into a patch.
Comment 2 Daniel Colascione 2017-10-24 16:42:32 UTC
Thanks!