Created attachment 338069 [details] [review]
Don't free (transfer none) GValues when converting from Python

Otherwise the GValue will become invalid once the called function returns.

Shouldn't it still be freed in case the passed in Python object is not a GValue and a temp GValue gets created?

(In reply to Christoph Reiter (lazka) from comment #2)
> Shouldn't it still be freed in case the passed in Python object is not a
> GValue and a temp GValue gets created?

copy_reference in pygi_arg_gvalue_from_py_marshal() will always be TRUE from what I can tell. Which would indicate to me that:

@ +120
> } else {
>     value = g_slice_new0 (GValue);
>     g_value_init (value, G_VALUE_TYPE (source_value));
>     g_value_copy (source_value, value);
> }

is all dead code.

There is still the second place where the local allocation occurs:

@ +126
> value = g_slice_new0 (GValue);
> g_value_init (value, object_type);
> if (pyg_value_from_pyobject (value, py_arg) < 0) {

and that does seem like it would be leaking to me.

What about adding an out parameter to arg_struct_from_py_marshal_adapter() that determines whether or not cleanup_data should be set to v_pointer?

(In reply to Christoph Reiter (lazka) from comment #2)
> Shouldn't it still be freed in case the passed in Python object is not a
> GValue and a temp GValue gets created?

The issue is that the GValue shouldn't be freed because the function is taking ownership of the GValue (newly created or not). However, I didn't check passing a GValue as I ran into another issue.

The issue I ran into is that (transfer full) on a GValue is fairly unsafe. This is due to the fact that GValue doesn't have a constructor which means it could be allocated with g_new() or g_slice_new(). Thus when the called function goes to cleanup the passed in GValue it could use g_free() or g_slice_free() and cause various issues.

This patch still avoids a double free bug (at least when passing a non-GValue), it would be good to add a test in GI and further investigate this issue.

Looking into the second issue it seems glib, gtk+ and lgi all use g_new() for allocating GValues so switching might be a good fix.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/124.