After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769245 - is_valid_heap_iter define misses NULL pointer check
is_valid_heap_iter define misses NULL pointer check
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gvariant
unspecified
Other Linux
: Normal minor
: ---
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-07-27 21:37 UTC by Yury Usishchev
Modified: 2016-08-03 15:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (930 bytes, patch)
2016-07-27 21:47 UTC, Yury Usishchev
committed Details | Review

Description Yury Usishchev 2016-07-27 21:37:11 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.
Comment 1 Yury Usishchev 2016-07-27 21:47:12 UTC
Created attachment 332241 [details] [review]
proposed patch

Proposed patch attached
Comment 2 Matthias Clasen 2016-07-29 15:44:06 UTC
Is this causing you any problem ?
Comment 3 Yury Usishchev 2016-08-01 12:02:42 UTC
(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.
Comment 4 Matthias Clasen 2016-08-03 12:17:28 UTC
(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
Comment 5 Colin Walters 2016-08-03 14:04:14 UTC
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/
Comment 6 Colin Walters 2016-08-03 15:26:39 UTC
Review of attachment 332241 [details] [review]:

LGTM