GNOME Bugzilla – Bug 640807
improve GVariant behaviour with invalid pointers
Last modified: 2011-01-28 13:27:06 UTC
Created attachment 179498 [details] Simple C program with a ref counting error causing g_variant_ref_sink to deadlock Attaching a trivial program with a *very common* ref counting error on GVariant that causes a deadlock in g_variant_ref_sink. Of course this is a client side fail, but still, it's not a very friendly way to report errors on a piece of code you wouldn't expect to deadlock.
I'm not sure what to say.... You're making use of memory that has been returned to the allocator. The behaviour here is therefore dependent on what the allocator is doing with that memory. Of course, we make the issue that you report a bit more likely to appear by the object being locked when we return it to the allocator. I've always thought of that as a bit of a feature: it's my opinion that obvious memory corruption bugs are better than subtle ones. I suppose the best behaviour here would be an abort or a straight segmentation fault, if we could get that to happen. We probably could encourage that by leaving the GVariant in a state that is likely to cause an abort (if used) when we return it to the allocator (and hope that that state is still what's left when you try to access it next time).
I agree more or less. I just would really like to see an assert() or g_critical() at minimum. Consider the case where this happens in a WM or other managed process. If it crashes it can be restarted; if it just hangs there's not much the system can do to help the end user. Maybe also a check if ref_count is > 0. It's not sure fire by any means, a slight hack perhaps, but it will increase the chances of getting good debugging output, and be solid and cheap in the positive case.
commit d4209c1c415766c8735eb08500cd7de450c7c09a Author: Ryan Lortie <desrt@desrt.ca> Date: Fri Jan 28 08:23:11 2011 -0500 GVariant: clear memory before releasing it Bug #640807 makes a reasonable case for why it's better to have your program crash outright in the case of memory errors. With this modification, GVariant is far more likely to do that in the case that a GVariant pointer is used shortly after being freed. Basically, I bzero() the struct just before returning it to the slice allocator. No promises for what the slice allocator does after that, of course, but it should improve some 'common' cases. Now we hit this assertion failure: GLib:ERROR:gvarianttypeinfo.c:186:g_variant_type_info_check: assertion failed: (0 <= index && index < 24) instead of having the deadlock.