GNOME Bugzilla – Bug 769245
is_valid_heap_iter define misses NULL pointer check
Last modified: 2016-08-03 15:28:07 UTC
In glib/gvariant.c:2917: #define is_valid_iter(i) (i != NULL && \ GVSI(i)->magic == GVSI_MAGIC) #define is_valid_heap_iter(i) (GVHI(i)->magic == GVHI_MAGIC && \ is_valid_iter(i)) is_valie_iter have NULL pointer check, but is_valid_heap_iter dereferences pointer and only then checks if it is NULL.
Created attachment 332241 [details] [review] proposed patch Proposed patch attached
Is this causing you any problem ?
(In reply to Matthias Clasen from comment #2) > Is this causing you any problem ? I cannot say for sure. I have some application that communicates over dbus and iterates over reply with g_variant_get(reply_var, type_str, &iter); while(g_variant_iter_loop(iter, ...)){ ... } g_variant_iter_free(iter); It is extremely hard to get there with any debugger and check in runtime what is going on. But there is an Address Sanitizer report of heap-buffer-overflow which happens in this particular macro, on "GVHI(i)->magic == GVHI_MAGIC" string. Buffer is allocated somewhere in g_variant_get and obviously is connected with iter. I am not sure what is the cause of this report. Maybe g_variant_iter_free is not needed at all in this situation, I cannot check that. But if I apply this patch, ASan report is not reproduced. And I think it is logical that if there is NULL-pointer check it should go before dereference of that pointer.
(In reply to Yury Usishchev from comment #3) > > And I think it is logical that if there is NULL-pointer check it should go > before dereference of that pointer. All those null pointer checks are just a curtesy - we tell you that you've made a programming error. But in terms of the API contract, they're no different from a crash
We do need to be careful with checking for NULL before deferencing it. Deref-before-NULL is undefined behavior and GCC is within its rights to entirely omit the code for example. https://lwn.net/Articles/342330/
Review of attachment 332241 [details] [review]: LGTM