GNOME Bugzilla – Bug 769064
Assert that array size does not change during free handlers
Last modified: 2018-04-20 13:03:08 UTC
In bug #769061 it was possible that a programmer might accidentally mutate a pointer array during an element free handler. Assert that this does not happen to alert the programmer that this case is not supported.
Created attachment 331944 [details] [review] garray: Assert that array size does not change during free handlers
One quick nit is that the style doesn't match the rest of the file for scoping. Another idea might be to add an "in_destruction" bit to the structure (or steal the pdata pointer) so that we can g_return_if_fail() in mutation functions and index accessors. That would allow us to know exactly where the problem is with G_DEBUG=fatal-criticals to get a stack trace.
(In reply to Christian Hergert from comment #2) > One quick nit is that the style doesn't match the rest of the file for > scoping. > Ah, of course. Thanks for pointing that out. > Another idea might be to add an "in_destruction" bit to the structure (or > steal the pdata pointer) so that we can g_return_if_fail() in mutation > functions and index accessors. That would allow us to know exactly where the > problem is with G_DEBUG=fatal-criticals to get a stack trace. I suppose that is the better approach. I'll rewrite the patch on this suggestion. Thanks for the review!
Any progress on this, Sam?
(In reply to Philip Withnall from comment #4) > Any progress on this, Sam? Not at the moment. I'll take a look tomorrow.
Created attachment 371156 [details] [review] garray: Steal segment during destruction And warn in other parts of the code if the caller attempts to change the array bounds during destruction, this is not a valid operation.
Review of attachment 371156 [details] [review]: I’ll push with the change below, thanks. ::: glib/garray.c @@ +1116,3 @@ + } + + rarray->pdata = stolen_pdata; I don’t think there’s any need to reassign the stolen_pdata to rarray->pdata here: we immediately set it to NULL or free the rarray.
Review of attachment 371156 [details] [review]: ::: glib/garray.c @@ +1112,3 @@ if (rarray->element_free_func != NULL) + { + for (gsize i = 0; i < rarray->len; ++i) Also, initial declarations in `for` loops are not allowed in GLib because some of the compilers we target don’t support them. :-( I’ll fix this too.
Pushed with those fixups. Attachment 371156 [details] pushed as 0e1a26d - garray: Steal segment during destruction