GNOME Bugzilla – Bug 599197
array ref and unref functions crash on NULL array.
Last modified: 2010-02-03 20:25:58 UTC
Created attachment 145955 [details] Adds g_return_(val_)if_fail clauses before dereferencing a pointer happens. As in title - there is a segfault when NULL is passed to: g_array_ref, g_array_unref, g_ptr_array_ref, g_ptr_array_unref. (Not mentioning g_byte_array_ref or unref, since it calls g_array_ref and unref). Simple test case is: #include <glib.h> int main(void) { GPtrArray* null_array = NULL; g_ptr_array_ref(null_array); return 0; } I think g_return_(val_)if_fail() clauses should be added before any dereferencing an array happens. I'll attach a patch.
No, it's supposed to work this way. Similarly, g_object_unref() and many other unref functions are not NULL-safe. Suggest to close as NOTABUG.
So it is better for GLib to crash instead of display warnings, that could be more helpful than sudden `segmentation fault' message in terminal? Since passing NULL to ref functions is clearly programming error, it would be useful to have such warnings.
Comment on attachment 145955 [details] Adds g_return_(val_)if_fail clauses before dereferencing a pointer happens. (Please mark patches as text/plain so it is easier to read them from the browser. Thanks.)
(In reply to comment #2) > So it is better for GLib to crash instead of display warnings, that could be > more helpful than sudden `segmentation fault' message in terminal? Since > passing NULL to ref functions is clearly programming error, it would be useful > to have such warnings. Oh, sorry, I assumed you wanted to handle passing NULL without a warning (the bug title kinda indicates that) and I didn't read the patch. My bad. I agree it would be useful with such warnings.
(In reply to comment #3) > (From update of attachment 145955 [details]) > (Please mark patches as text/plain so it is easier to read them from the > browser. Thanks.) (I was using form in https://bugzilla.gnome.org/enter_bug.cgi?product=glib and there was no option about type of attachment, only path and description. Next time I'll check after addition if type is proper one.) (In reply to comment #4) > Oh, sorry, I assumed you wanted to handle passing NULL without a warning (the > bug title kinda indicates that) and I didn't read the patch. My bad. I agree it > would be useful with such warnings. Yeah, I see now that topic is quite poorly worded. What's more, there are some public functions in garray.c that are not NULL-safe (that is - have no g_return_(val_)if_fail, but go on with dereferencing unchecked pointers): g_array_get_element_size(), g_array_append_vals(), g_array_prepend_vals(), g_array_insert_vals(), g_array_set_size(), g_ptr_array_set_free_func() Even checks are in some place inconsistent (array vs. array != NULL), but it's nitpicking.
The patch looks fine to me, both for stable and for master (as this patch only makes the API behave a bit more consistent). There are no compile-warnings introduced. Matthias, can I commit this one?
I probably should make a patch adding such statements to all function mentioned by me in comment 5.
Created attachment 151351 [details] [review] Patch adding g_return_(val_)if_fail in more functions. This puts g_return_(val_)if_fail in every public function in arrays.
(In reply to comment #8) > Created an attachment (id=151351) [details] [review] > Patch adding g_return_(val_)if_fail in more functions. > > This puts g_return_(val_)if_fail in every public function in arrays. Kresimir, one thing I'd like to add here. If you fix bugs with a bug id, please put a line like "Fixes: Bug 599197 - array ref and unref functions crash on NULL array" as the third line of your commit message (that's explaining a lot more than just a link to the bug). Then people will have a simple way of finding out why a change was made (as it's not always as obvious as here). Also, please add a more verbose description of _why_ you did that change. Something along the lines of "Made sure all the functions have safety-guards to protect against being called with NULL arguments". Please consider to fix these issues (you can trivially do this with "git commit --amend").
Created attachment 151357 [details] [review] Patch adding g_return_(val_)if_fail in more functions - better wording version. Now is good? I usually do a git log before git commit to see what kind of commit messages are used in project. Seems sometimes I got it wrong.
(In reply to comment #10) > Created an attachment (id=151357) [details] [review] > Patch adding g_return_(val_)if_fail in more functions - better wording version. > > Now is good? I usually do a git log before git commit to see what kind of > commit messages are used in project. Seems sometimes I got it wrong. Looks fine to me. Do you have commit access to glib?
I do, so I pushed it to master.