Commit 4623caa7 authored by Simon Feltman's avatar Simon Feltman
Browse files

Fix GValue array marshaling leaks and crash fallout

* Decrement references for results of PySequence_GetItem. There were a few
places we were not decrementing the Python reference, leaking the value.
* Add tracking of Python arguments with recursive marshaling cleanup. This
allows arrays of GValues which have been coerced from Python types to be
properly free'd (also fixes bug 703662).
* Use g_variant_ref for variant arguments marked as transfer everything.
This fixes double free's caused by the decrementing of PySequence_GetItem
results.

https://bugzilla.gnome.org/show_bug.cgi?id=693402
parent 549f849e
......@@ -45,6 +45,7 @@ typedef PyObject *(*PyGIMarshalToPyFunc) (PyGIInvokeState *state,
typedef void (*PyGIMarshalCleanupFunc) (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg, /* always NULL for to_py cleanup */
gpointer data,
gboolean was_processed);
......
......@@ -579,6 +579,7 @@ _invoke_marshal_out_args (PyGIInvokeState *state, PyGICallableCache *cache)
if (to_py_cleanup != NULL)
to_py_cleanup ( state,
cache->return_cache,
NULL,
&state->return_arg,
FALSE);
}
......
......@@ -24,6 +24,7 @@
static inline void
_cleanup_caller_allocates (PyGIInvokeState *state,
PyGIArgCache *cache,
PyObject *py_obj,
gpointer data,
gboolean was_processed)
{
......@@ -97,16 +98,18 @@ pygi_marshal_cleanup_args_from_py_marshal_success (PyGIInvokeState *state,
for (i = 0; i < _pygi_callable_cache_args_len (cache); i++) {
PyGIArgCache *arg_cache = _pygi_callable_cache_get_arg (cache, i);
PyGIMarshalCleanupFunc cleanup_func = arg_cache->from_py_cleanup;
PyObject *py_arg = PyTuple_GET_ITEM (state->py_in_args,
arg_cache->py_arg_index);
if (cleanup_func &&
arg_cache->direction == PYGI_DIRECTION_FROM_PYTHON &&
state->args[i]->v_pointer != NULL)
cleanup_func (state, arg_cache, state->args[i]->v_pointer, TRUE);
cleanup_func (state, arg_cache, py_arg, state->args[i]->v_pointer, TRUE);
if (cleanup_func &&
arg_cache->direction == PYGI_DIRECTION_BIDIRECTIONAL &&
state->args_data[i] != NULL) {
cleanup_func (state, arg_cache, state->args_data[i], TRUE);
cleanup_func (state, arg_cache, py_arg, state->args_data[i], TRUE);
}
}
}
......@@ -122,6 +125,7 @@ pygi_marshal_cleanup_args_to_py_marshal_success (PyGIInvokeState *state,
if (cleanup_func && state->return_arg.v_pointer != NULL)
cleanup_func (state,
cache->return_cache,
NULL,
state->return_arg.v_pointer,
TRUE);
}
......@@ -136,11 +140,13 @@ pygi_marshal_cleanup_args_to_py_marshal_success (PyGIInvokeState *state,
if (cleanup_func != NULL && data != NULL)
cleanup_func (state,
arg_cache,
NULL,
data,
TRUE);
else if (arg_cache->is_caller_allocates && data != NULL) {
_cleanup_caller_allocates (state,
arg_cache,
NULL,
data,
TRUE);
}
......@@ -162,18 +168,22 @@ pygi_marshal_cleanup_args_from_py_parameter_fail (PyGIInvokeState *state,
PyGIArgCache *arg_cache = _pygi_callable_cache_get_arg (cache, i);
PyGIMarshalCleanupFunc cleanup_func = arg_cache->from_py_cleanup;
gpointer data = state->args[i]->v_pointer;
PyObject *py_arg = PyTuple_GET_ITEM (state->py_in_args,
arg_cache->py_arg_index);
if (cleanup_func &&
arg_cache->direction == PYGI_DIRECTION_FROM_PYTHON &&
data != NULL) {
cleanup_func (state,
arg_cache,
py_arg,
data,
i < failed_arg_index);
} else if (arg_cache->is_caller_allocates && data != NULL) {
_cleanup_caller_allocates (state,
arg_cache,
py_arg,
data,
FALSE);
}
......@@ -198,6 +208,7 @@ pygi_marshal_cleanup_args_to_py_parameter_fail (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_utf8 (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -210,6 +221,7 @@ _pygi_marshal_cleanup_from_py_utf8 (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_utf8 (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......@@ -223,6 +235,7 @@ _pygi_marshal_cleanup_to_py_utf8 (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_interface_object (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -236,6 +249,7 @@ _pygi_marshal_cleanup_from_py_interface_object (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_interface_object (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......@@ -249,6 +263,7 @@ _pygi_marshal_cleanup_to_py_interface_object (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_interface_callback (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -262,15 +277,18 @@ _pygi_marshal_cleanup_from_py_interface_callback (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_interface_struct_gvalue (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
if (was_processed) {
PyObject *py_arg = PyTuple_GET_ITEM (state->py_in_args,
arg_cache->py_arg_index);
/* Note py_arg can be NULL for hash table which is a bug. */
if (was_processed && py_arg != NULL) {
GType py_object_type =
pyg_type_from_object_strict ( (PyObject *) py_arg->ob_type, FALSE);
/* When a GValue was not passed, it means the marshalers created a new
* one to pass in, clean this up.
*/
if (py_object_type != G_TYPE_VALUE) {
g_value_unset ((GValue *) data);
g_slice_free (GValue, data);
......@@ -281,6 +299,7 @@ _pygi_marshal_cleanup_from_py_interface_struct_gvalue (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_interface_struct_foreign (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -293,6 +312,7 @@ _pygi_marshal_cleanup_from_py_interface_struct_foreign (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_interface_struct_foreign (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......@@ -336,6 +356,7 @@ _wrap_c_array (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_array (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -367,6 +388,7 @@ _pygi_marshal_cleanup_from_py_array (PyGIInvokeState *state,
for (i = 0; i < len; i++) {
gpointer item;
PyObject *py_item = NULL;
/* case 1: GPtrArray */
if (ptr_array_ != NULL)
......@@ -387,7 +409,9 @@ _pygi_marshal_cleanup_from_py_array (PyGIInvokeState *state,
}
}
cleanup_func (state, sequence_cache->item_cache, item, TRUE);
py_item = PySequence_GetItem (py_arg, i);
cleanup_func (state, sequence_cache->item_cache, py_item, item, TRUE);
Py_XDECREF (py_item);
}
}
......@@ -407,6 +431,7 @@ _pygi_marshal_cleanup_from_py_array (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_array (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......@@ -438,6 +463,7 @@ _pygi_marshal_cleanup_to_py_array (PyGIInvokeState *state,
for (i = 0; i < len; i++) {
cleanup_func (state,
sequence_cache->item_cache,
NULL,
(array_ != NULL) ? g_array_index (array_, gpointer, i) : g_ptr_array_index (ptr_array_, i),
was_processed);
}
......@@ -453,6 +479,7 @@ _pygi_marshal_cleanup_to_py_array (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_glist (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -467,12 +494,17 @@ _pygi_marshal_cleanup_from_py_glist (PyGIInvokeState *state,
PyGIMarshalCleanupFunc cleanup_func =
sequence_cache->item_cache->from_py_cleanup;
GSList *node = list_;
gsize i = 0;
while (node != NULL) {
PyObject *py_item = PySequence_GetItem (py_arg, i);
cleanup_func (state,
sequence_cache->item_cache,
py_item,
node->data,
TRUE);
Py_XDECREF (py_item);
node = node->next;
i++;
}
}
......@@ -496,6 +528,7 @@ _pygi_marshal_cleanup_from_py_glist (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_glist (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......@@ -513,6 +546,7 @@ _pygi_marshal_cleanup_to_py_glist (PyGIInvokeState *state,
while (node != NULL) {
cleanup_func (state,
sequence_cache->item_cache,
NULL,
node->data,
was_processed);
node = node->next;
......@@ -537,6 +571,7 @@ _pygi_marshal_cleanup_to_py_glist (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_from_py_ghash (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed)
{
......@@ -566,11 +601,13 @@ _pygi_marshal_cleanup_from_py_ghash (PyGIInvokeState *state,
if (key != NULL && key_cleanup_func != NULL)
key_cleanup_func (state,
hash_cache->key_cache,
NULL,
key,
TRUE);
if (value != NULL && value_cleanup_func != NULL)
value_cleanup_func (state,
hash_cache->value_cache,
NULL,
value,
TRUE);
}
......@@ -587,6 +624,7 @@ _pygi_marshal_cleanup_from_py_ghash (PyGIInvokeState *state,
void
_pygi_marshal_cleanup_to_py_ghash (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed)
{
......
......@@ -42,58 +42,72 @@ void pygi_marshal_cleanup_args_to_py_parameter_fail (PyGIInvokeState *state,
void _pygi_marshal_cleanup_from_py_utf8 (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_utf8 (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_interface_struct_gvalue (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_interface_struct_foreign (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_interface_struct_foreign (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_interface_object (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_interface_object (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_interface_callback (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_array (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_array (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_glist (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_glist (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_from_py_ghash (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *py_arg,
gpointer data,
gboolean was_processed);
void _pygi_marshal_cleanup_to_py_ghash (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
PyObject *dummy,
gpointer data,
gboolean was_processed);
G_END_DECLS
......
......@@ -788,9 +788,12 @@ _pygi_marshal_from_py_array (PyGIInvokeState *state,
callable_cache,
sequence_cache->item_cache,
py_item,
&item))
&item)) {
Py_DECREF (py_item);
goto err;
}
Py_DECREF (py_item);
/* FIXME: it is much more efficent to have seperate marshaller
* for ptr arrays than doing the evaluation
* and casting each loop iteration
......@@ -827,7 +830,7 @@ _pygi_marshal_from_py_array (PyGIInvokeState *state,
/* we free the original copy already, the new one is a plain struct
* in an array. _pygi_marshal_cleanup_from_py_array() does not free it again */
if (from_py_cleanup)
from_py_cleanup (state, item_arg_cache, item.v_pointer, TRUE);
from_py_cleanup (state, item_arg_cache, py_item, item.v_pointer, TRUE);
} else if (!is_boxed) {
/* HACK: Gdk.Atom is merely an integer wrapped in a pointer,
* so we must not dereference it; just copy the pointer
......@@ -841,7 +844,7 @@ _pygi_marshal_from_py_array (PyGIInvokeState *state,
memcpy (array_->data + (i * item_size), item.v_pointer, item_size);
if (from_py_cleanup)
from_py_cleanup (state, item_arg_cache, item.v_pointer, TRUE);
from_py_cleanup (state, item_arg_cache, py_item, item.v_pointer, TRUE);
}
} else if (is_boxed && !item_iface_cache->arg_cache.is_pointer) {
/* The array elements are not expected to be pointers, but the
......@@ -860,6 +863,7 @@ _pygi_marshal_from_py_array (PyGIInvokeState *state,
} else {
g_array_insert_val (array_, i, item);
}
continue;
err:
if (sequence_cache->item_cache->from_py_cleanup != NULL) {
......@@ -868,10 +872,13 @@ err:
sequence_cache->item_cache->from_py_cleanup;
for(j = 0; j < i; j++) {
PyObject *py_item = PySequence_GetItem (py_arg, j);
cleanup_func (state,
sequence_cache->item_cache,
py_item,
g_array_index (array_, gpointer, j),
TRUE);
Py_DECREF (py_item);
}
}
......@@ -975,6 +982,7 @@ _pygi_marshal_from_py_glist (PyGIInvokeState *state,
&item))
goto err;
Py_DECREF (py_item);
list_ = g_list_prepend (list_, _pygi_arg_to_hash_pointer (&item, sequence_cache->item_cache->type_tag));
continue;
err:
......@@ -983,6 +991,7 @@ err:
PyGIMarshalCleanupFunc cleanup = sequence_cache->item_cache->from_py_cleanup;
}
*/
Py_DECREF (py_item);
g_list_free (list_);
_PyGI_ERROR_PREFIX ("Item %i: ", i);
return FALSE;
......@@ -1042,6 +1051,7 @@ _pygi_marshal_from_py_gslist (PyGIInvokeState *state,
&item))
goto err;
Py_DECREF (py_item);
list_ = g_slist_prepend (list_, _pygi_arg_to_hash_pointer (&item, sequence_cache->item_cache->type_tag));
continue;
err:
......@@ -1051,6 +1061,7 @@ err:
}
*/
Py_DECREF (py_item);
g_slist_free (list_);
_PyGI_ERROR_PREFIX ("Item %i: ", i);
return FALSE;
......@@ -1757,6 +1768,9 @@ _pygi_marshal_from_py_interface_struct (PyObject *py_arg,
return FALSE;
}
arg->v_pointer = pyg_pointer_get (py_arg, void);
if (transfer == GI_TRANSFER_EVERYTHING) {
g_variant_ref ((GVariant *)arg->v_pointer);
}
} else {
PyErr_Format (PyExc_NotImplementedError,
......
......@@ -425,6 +425,7 @@ err:
for (j = processed_items; j < array_->len; j++) {
cleanup_func (state,
seq_cache->item_cache,
NULL,
g_array_index (array_, gpointer, j),
FALSE);
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment