atk-adaptor/bridge: Fix GList handling resulting in memory corruption

As pointed out by this valgrind log:

==2809== Thread 1:
==2809== Invalid write of size 8
==2809==    at 0x18FCF001: remove_events (bridge.c:759)
==2809==    by 0x18FCF001: handle_event_listener_deregistered (bridge.c:788)
==2809==    by 0x18FCF001: signal_filter (bridge.c:827)
==2809==    by 0x200ECDFD: dbus_connection_dispatch (dbus-connection.c:4631)
==2809==    by 0x1FEBD0F4: ??? (in /usr/lib64/libatspi.so.0.0.1)
==2809==    by 0xFD8D4C8: g_main_dispatch (gmain.c:3201)
==2809==    by 0xFD8D4C8: g_main_context_dispatch (gmain.c:3854)
==2809==    by 0xFD8D817: g_main_context_iterate.isra.21 (gmain.c:3927)
==2809==    by 0xFD8DAE9: g_main_loop_run (gmain.c:4123)
==2809==    by 0xDFF84B4: gtk_main (in /usr/lib64/libgtk-3.so.0.2200.10)
==2809==    by 0x403DE0: main (in /usr/bin/evolution)
==2809==  Address 0x29f22540 is 16 bytes inside a block of size 24 free'd
==2809==    at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==2809==    by 0xFD92BCD: g_free (gmem.c:189)
==2809==    by 0xFDAA518: g_slice_free1 (gslice.c:1136)
==2809==    by 0xFD89463: g_list_remove (glist.c:521)
==2809==    by 0x18FCF000: remove_events (bridge.c:759)
==2809==    by 0x18FCF000: handle_event_listener_deregistered (bridge.c:788)
==2809==    by 0x18FCF000: signal_filter (bridge.c:827)
==2809==    by 0x200ECDFD: dbus_connection_dispatch (dbus-connection.c:4631)
==2809==    by 0x1FEBD0F4: ??? (in /usr/lib64/libatspi.so.0.0.1)
==2809==    by 0xFD8D4C8: g_main_dispatch (gmain.c:3201)
==2809==    by 0xFD8D4C8: g_main_context_dispatch (gmain.c:3854)
==2809==    by 0xFD8D817: g_main_context_iterate.isra.21 (gmain.c:3927)
==2809==    by 0xFD8DAE9: g_main_loop_run (gmain.c:4123)
==2809==    by 0xDFF84B4: gtk_main (in /usr/lib64/libgtk-3.so.0.2200.10)
==2809==    by 0x403DE0: main (in /usr/bin/evolution)
==2809==  Block was alloc'd at
==2809==    at 0x4C29BE3: malloc (vg_replace_malloc.c:299)
==2809==    by 0xFD92ABD: g_malloc (gmem.c:94)
==2809==    by 0xFDA9EFD: g_slice_alloc (gslice.c:1025)
==2809==    by 0xFD89983: g_list_append (glist.c:261)
==2809==    by 0x18FCE7EE: add_event (bridge.c:80)
==2809==    by 0x18FCE7EE: add_event_from_iter (bridge.c:217)
==2809==    by 0x18FCEEF6: handle_event_listener_registered (bridge.c:721)
==2809==    by 0x18FCEEF6: signal_filter (bridge.c:825)
==2809==    by 0x200ECDFD: dbus_connection_dispatch (dbus-connection.c:4631)
==2809==    by 0x1FEBD0F4: ??? (in /usr/lib64/libatspi.so.0.0.1)
==2809==    by 0xFD8D4C8: g_main_dispatch (gmain.c:3201)
==2809==    by 0xFD8D4C8: g_main_context_dispatch (gmain.c:3854)
==2809==    by 0xFD8D817: g_main_context_iterate.isra.21 (gmain.c:3927)
==2809==    by 0xFD8DAE9: g_main_loop_run (gmain.c:4123)
==2809==    by 0xDFF84B4: gtk_main (in /usr/lib64/libgtk-3.so.0.2200.10)

This line:

list->prev = g_list_remove (list->prev, evdata);

writes over free'd memory since the list link pointed to by the 'list'
pointer is free'd by g_list_remove(). We can use g_list_delete_link()
instead to achieve the intended result (and not re-iterate the whole
list) with less code overall.

Thanks to Milan Crha <mcrha@redhat.com> for investigating and
providing the valgring log.

https://bugzilla.gnome.org/show_bug.cgi?id=781658

diff --git a/atk-adaptor/bridge.c b/atk-adaptor/bridge.c
index 7de84d4669d59b1d4dcb2a088944b65743dfe65f..0b2b7362d8876f636942d558df4c2897c386a285 100644 (file)
--- a/atk-adaptor/bridge.c
+++ b/atk-adaptor/bridge.c
@@ -748,22 +748,17 @@ remove_events (const char *bus_name, const char *event)
       if (!g_strcmp0 (evdata->bus_name, bus_name) &&
           spi_event_is_subtype (evdata->data, remove_data))
         {
+          GList *next;
           GList *events = spi_global_app_data->events;
+
           g_strfreev (evdata->data);
           g_free (evdata->bus_name);
           g_slist_free_full (evdata->properties, free_property_definition);
           g_free (evdata);
-          if (list->prev)
-            {
-              GList *next = list->next;
-              list->prev = g_list_remove (list->prev, evdata);
-              list = next;
-            }
-          else
-            {
-              spi_global_app_data->events = g_list_remove (events, evdata);
-              list = spi_global_app_data->events;
-            }
+
+          next = list->next;
+          spi_global_app_data->events = g_list_delete_link (events, list);
+          list = next;
         }
       else
         {