GNOME Bugzilla – Bug 667167
Use-after-free in gconf_engine_get_fuller
Last modified: 2012-10-22 11:52:12 UTC
I got a nice valgrind log from evolution at [1] showing a use-after-free in gconf_engine_get_fuller, thus I'm moving it here, to be fixed properly. [1] https://bugzilla.redhat.com/show_bug.cgi?id=768973#c15 Invalid read of size 1 at 0x435A6477: gconf_engine_get_fuller (gconf-dbus.c:1245) by 0x435A6617: gconf_engine_get_entry (gconf-dbus.c:1288) by 0x435A01D8: get (gconf-client.c:1493) by 0x435A29CD: gconf_client_get_full.constprop.11 (gconf-client.c:1543) by 0x435A2D4F: gconf_client_get_list (gconf-client.c:1912) by 0x431E3AD: gconf_accounts_changed (e-account-list.c:135) by 0x431EA82: e_account_list_construct (e-account-list.c:262) by 0x431EB1C: e_account_list_new (e-account-list.c:238) by 0x415C10DE: e_get_account_list (e-account-utils.c:58) by 0x6EB7617: em_folder_tree_model_init (em-folder-tree-model.c:519) by 0x42031B7A: g_type_create_instance (gtype.c:1885) by 0x42010EDB: g_object_constructor (gobject.c:1629) by 0x42013FF8: g_object_newv (gobject.c:1412) by 0x42014B4F: g_object_new (gobject.c:1322) by 0x6EB76F4: em_folder_tree_model_new (em-folder-tree-model.c:534) by 0x6EB7724: em_folder_tree_model_get_default (em-folder-tree-model.c:543) by 0x6E7677A: mail_backend_constructed (e-mail-backend.c:769) by 0x70615AF: mail_shell_backend_constructed (e-mail-shell-backend.c:376) by 0x42013F51: g_object_newv (gobject.c:1521) by 0x420148DA: g_object_new_valist (gobject.c:1610) by 0x5803E9F: ??? Address 0x7867968 is 10,512 bytes inside a block of size 10,568 free'd at 0x4029EED: free (vg_replace_malloc.c:366) by 0x421D3ECF: dbus_free (in /lib/libdbus-1.so.3.5.6) by 0x421D4784: ??? (in /lib/libdbus-1.so.3.5.6) by 0x421C09FE: ??? (in /lib/libdbus-1.so.3.5.6) by 0x435A62AA: gconf_engine_get_fuller (gconf-dbus.c:1227) by 0x435A6617: gconf_engine_get_entry (gconf-dbus.c:1288) by 0x435A01D8: get (gconf-client.c:1493) by 0x435A29CD: gconf_client_get_full.constprop.11 (gconf-client.c:1543) by 0x435A2D4F: gconf_client_get_list (gconf-client.c:1912) by 0x431E3AD: gconf_accounts_changed (e-account-list.c:135) by 0x431EA82: e_account_list_construct (e-account-list.c:262) by 0x431EB1C: e_account_list_new (e-account-list.c:238) by 0x415C10DE: e_get_account_list (e-account-utils.c:58) by 0x6EB7617: em_folder_tree_model_init (em-folder-tree-model.c:519) by 0x42031B7A: g_type_create_instance (gtype.c:1885) by 0x42010EDB: g_object_constructor (gobject.c:1629) by 0x42013FF8: g_object_newv (gobject.c:1412) by 0x42014B4F: g_object_new (gobject.c:1322) by 0x6EB76F4: em_folder_tree_model_new (em-folder-tree-model.c:534) by 0x6EB7724: em_folder_tree_model_get_default (em-folder-tree-model.c:543) by 0x6E7677A: mail_backend_constructed (e-mail-backend.c:769) by 0x70615AF: mail_shell_backend_constructed (e-mail-shell-backend.c:376) by 0x42013F51: g_object_newv (gobject.c:1521) by 0x420148DA: g_object_new_valist (gobject.c:1610) by 0x5803E9F: ???
Created attachment 204488 [details] [review] proposed gconf-3.2.3-use-after-free patch for GConf-3.2.3; This fixes the issue. The problem is that returned string pointers are owned by the DBus message, thus when the message is freed the strings are freed too. The patch advertises strings as 'const' and frees the dbus message only after a copy of the string is created.
Created attachment 204490 [details] [review] [PATCH] gconf-dbus: Avoid use after free Bug originating from: http://bugzilla.redhat.com/show_bug.cgi?id=768973#c15 Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=667167 --- gconf/gconf-dbus.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Review of attachment 204488 [details] [review]: Looks good, please commit to git.
Review of attachment 204490 [details] [review]: Milan's patch is better.
The comments make not make sense. Milan, please push your patch.
Review of attachment 204488 [details] [review]: Hehe - a race condition when attaching a patch! :-) Milan, i'm not sure about advertising the schema_name as const - we want the caller of the function to free that memory after use. An out variable should only be const if that memory is owned elsewhere and is guaranteed to have a long life - e.g. it's static or owned by a invincible singleton.
Review of attachment 204488 [details] [review]: Agreed, sorry for not noticing that.
(In reply to comment #6) > Milan, i'm not sure about advertising the schema_name as const - we want the > caller of the function to free that memory after use. An out variable should > only be const if that memory is owned elsewhere and is guaranteed to have a > long life - e.g. it's static or owned by a invincible singleton. That's the thing, the function didn't allocate new memory, but returned the one from the dbus message, which is the owner. To indicate it I decided to declare the schema_name as const. Thinking that the function extracts data from the message it is more or less obvious that the memory is owned by the message. Will comment fix it or you need a g_strdup & g_free for them?
Created attachment 204555 [details] [review] updated patch for gconf-3.2.3; Using g_strdup() and g_free(), as requested. I agree, it makes internal API cleaner than the previous patch.
Ping Ross/Rob, the bug is still there, and the update patch too. It will be nice to have this fixed.
(In reply to comment #10) > Ping Ross/Rob, the bug is still there, and the update patch too. It will be > nice to have this fixed. I'm happy with this version of the patch - and will land it after Monday.
(In reply to comment #11) > I'm happy with this version of the patch - and will land it after Monday. Rob: Did this get in? Can this get closed?
*** Bug 664907 has been marked as a duplicate of this bug. ***
Is now.
(In reply to comment #14) > Is now. … in gconf and is commit 84884e9d [1]. [1] http://git.gnome.org/browse/gconf/commit/?id=84884e9df7ce8c081a1c223c66a799b82545ff1e