Created attachment 233187 [details] [review]
Patch.

On second thought, I'm not sure whether that patch is the best way of fixing it or if it makes sense to try to do something more generic, since there is a similar issue with C functions that return a GValue in a space allocated by the caller. However, in the latter case, pygi usually fetches the data from the value, so it is unlike the handling of boxes in that the allocated space is not needed once the value is marshalled, at least in many cases.

I'm afraid I'm not familiar with that part of the code and don't understand the patch that well, so sorry if this doesn't get a timely review.

I don't understand why the patch would manually set slice_allocated; pyg_boxed_new() properly allocates the boxed type with a constructor, so why should _boxed_dealloc() call g_slice_free1() instead of the proper destructor (g_boxed_free())?

The "|| arg_cache->is_caller_allocates" part makes sense, though.

Thanks!

pyg_boxed_new() only allocates the Python structure. It doesn't copy the C structure--it sets the reference to the C structure to the value passed into it. If is_caller_allocates is set, then the space for the C structure was allocated by pygi using g_slice_alloc. The free functions for boxed types generally call g_free() / assume that the boxed type was allocated by the copy function or by a corresponding _new function, although gtk_tree_iter_free calls g_slice_free, so in this case it wouldn't make a difference. In any case, to use the C type's destructor, then it and pygi would need to agree on the method in which the space was allocated, and this is not guaranteed to be the case as far as I can tell.

This would cause problems if a structure is passed using caller-allocated space and also contains elements that themselves need to be freed, requiring use of the type's destructor, although I'm not sure if there are any functions out there with arguments that fit that description, and it is not a case that would be specified well enough to be made to work sensibly.

*** Bug 686205 has been marked as a duplicate of this bug. ***

Thanks for the additional explanation!

(In reply to comment #4)
> This would cause problems if a structure is passed using caller-allocated space
> and also contains elements that themselves need to be freed, requiring use of
> the type's destructor

Right, that was my main concern as with this patch it would skip the g_boxed_free() call.

But indeed _boxed_new() (through _pygi_boxed_alloc()) would just plainly allocate the slice without a constructor, so at least for boxed types created from Python this should be okay.

I still can't say I'm 100% convinced about this patch, and it looks a bit ugly as it pokes internal object variables from "outside", but as far as I can see it should make things better, so let's get it in for now.

It seems this does cause a regression after all: When running the tests with G_SLICE=debug-blocks, we get 

GSlice: MemChecker: attempt to release non-allocated block: 0xc3f000 size=0

because this patch sets slice_allocated to TRUE, but doesn't set a corresponding size.

Reopening as we need a better solution for this.

Note, I got to this when debugging bug 693402: even with this patch we still get memory leaks with gtk_list_store_insert_with_values(), so it seems this patch doesn't even really fix that? How did you test this originally, with valgrind?

Thanks!

Reproducer:

$ G_SLICE=debug-blocks PYTHONPATH=. python3 -c 'from gi.repository import Gtk, GObject; st = Gtk.ListStore.new([GObject.TYPE_INT]); st.insert_with_valuesv(-1, [0], [42]); del st'
GSlice: MemChecker: attempt to release non-allocated block: 0x170d9c0 size=0

For the record, I added G_SLICE for "make check" by default in http://git.gnome.org/browse/pygobject/commit/?id=a51c72c771 and reverted the original patch here in http://git.gnome.org/browse/pygobject/commit/?id=70118c as it doesn't seem to help at all (it just tries to deallocate a slice of size 0).

While I was debugging this, I fixed a malloc vs. slice confusion and memleak related to boxed types: http://git.gnome.org/browse/pygobject/commit/?id=64bcca

But it turns out it's unrelated to this case, I still get this leak:

==8801== 32 bytes in 1 blocks are definitely lost in loss record 2,330 of 8,054
==8801==    at 0x4C2ABB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8801==    by 0x8047813: standard_calloc (gmem.c:104)
==8801==    by 0x80478A5: g_malloc0 (gmem.c:189)
==8801==    by 0x791FF5A: _caller_alloc (pygi-invoke.c:387)
==8801==    by 0x7920498: _invoke_marshal_in_args (pygi-invoke.c:485)
==8801==    by 0x7920E10: pygi_callable_info_invoke (pygi-invoke.c:659)
==8801==    by 0x7921014: _wrap_g_callable_info_invoke (pygi-invoke.c:685)
==8801==    by 0x4814AD: PyEval_EvalFrameEx (ceval.c:4374)
==8801==    by 0x47A90D: PyEval_EvalCodeEx (ceval.c:3433)
==8801==    by 0x4818A6: PyEval_EvalFrameEx (ceval.c:4160)
==8801==    by 0x47A90D: PyEval_EvalCodeEx (ceval.c:3433)
==8801==    by 0x47A9BA: PyEval_EvalCode (ceval.c:771)

This path goes to the "normal struct" code path, not the boxed types. That's a bit strange, as the documentation says that GtkTreeIters are also boxed types? I'll debug that now.

Ah, silly me; in gdb it shows that the code from comment 10 now does go through the "boxed" code path, and the memleak in comment 12 is precisely the one that got fixed by http://git.gnome.org/browse/pygobject/commit/?id=64bcca, i. e. for cleaning up the GtkTreeIter out argument.

I still get other leaks with Gtk.ListStore though, see bug 693402. These concern GValues, not boxed caller-allocated out arguments; but if I disable that part, the remaining leaks are due to this bug.

I seem to get better results (i. e. no remaining leaks apart from the GValue issues) with this modified patch which sets the size:

--- a/gi/pygi-marshal-to-py.c
+++ b/gi/pygi-marshal-to-py.c
@@ -815,7 +815,11 @@ _pygi_marshal_to_py_interface_struct (PyGIInvokeState   *state,
                                                               arg->v_pointer);
     } else if (g_type_is_a (type, G_TYPE_BOXED)) {
         py_obj = _pygi_boxed_new ( (PyTypeObject *)iface_cache->py_type, arg->v_pointer, 
-                                  arg_cache->transfer == GI_TRANSFER_EVERYTHING);
+                                  arg_cache->transfer == GI_TRANSFER_EVERYTHING || arg_cache->is_caller_allocates);
+        if (arg_cache->is_caller_allocates) {
+          ((PyGIBoxed *)py_obj)->slice_allocated = TRUE;
+          ((PyGIBoxed *)py_obj)->size = g_struct_info_get_size(iface_cache->interface_info);
+        }
     } else if (g_type_is_a (type, G_TYPE_POINTER)) {
         if (iface_cache->py_type == NULL ||
                 !PyType_IsSubtype ( (PyTypeObject *)iface_cache->py_type, &PyGIStruct_Type)) {

It's still a hack though, I'd rather keep this within pygi-boxed.c

Thank you!

Created attachment 237584 [details] [review]
Fix leak of caller-allocated boxed values

This patch is more intrusive, but I think it's cleaner as it keeps access to the internal data structures to pygi-boxed.c, and more importantly, requires the caller to think about how the passed gpointer was allocated; that way, we won't forget about this in other places.

Functionally it should be equivalent to Mike's original patch.

Mike, could you please verify that current pygobject master plus this patch improves the original leak that you were seeing? With which code did you test the original patch, i. e. in which case did it improve things?

@pygobject maintainers: review and opinions appreciated about this vs. the original patch with an extra size specification.

Review of attachment 237584 [details] [review]:

Moving the slice allocated info as an argument of _pygi_boxed_new is definitely cleaner.

Pushed, thanks for reviewing!

Fwiw, I'd been testing by running make check.valgrind; I'd found this bug while looking for bug 675726 (not relaizing at the time that it had already been filed). Yeah, I'd obviously missed something, and the new patch seems like an improvement.