EGdbusTemplates: Address crash on operation cancel

In situations when a synchronous operation was cancelled, but the response
was already piled in main context the client could receive two replies, one
from the reply, the other from the cancelled operation, effectively accessing
invalid memory in the second response. This may address also other similar
situations caused by cancelled operations.

diff --git a/libedataserver/e-gdbus-templates.c b/libedataserver/e-gdbus-templates.c
index 60306be..ef3e476 100644 (file)
--- a/libedataserver/e-gdbus-templates.c
+++ b/libedataserver/e-gdbus-templates.c
@@ -844,6 +844,7 @@ e_gdbus_complete_sync_method_uint (gpointer object,
 
 typedef struct _AsyncOpData
 {
+       gint ref_count;
        EGdbusAsyncOpKeeper *proxy;
        guint opid;
 
@@ -871,27 +872,37 @@ async_op_data_free (AsyncOpData *op_data)
 
        g_return_if_fail (op_data != NULL);
 
+       pending_ops = e_gdbus_async_op_keeper_get_pending_ops (op_data->proxy);
+
        if (op_data->cancel_idle_id) {
                GError *error = NULL;
 
                g_source_remove (op_data->cancel_idle_id);
                op_data->cancel_idle_id = 0;
 
+               if (pending_ops)
+                       g_hash_table_remove (pending_ops, GUINT_TO_POINTER (op_data->opid));
+
                if (!e_gdbus_async_op_keeper_cancel_op_sync (op_data->proxy, op_data->opid, NULL, &error)) {
                        g_debug ("%s: Failed to cancel operation: %s\n", G_STRFUNC, error ? error->message : "Unknown error");
                        g_clear_error (&error);
                }
+       } else if (pending_ops) {
+               g_hash_table_remove (pending_ops, GUINT_TO_POINTER (op_data->opid));
        }
 
        if (op_data->cancellable) {
-               if (op_data->cancel_id)
+               if (op_data->cancel_id) {
                        g_cancellable_disconnect (op_data->cancellable, op_data->cancel_id);
+                       op_data->cancel_id = 0;
+               }
                g_object_unref (op_data->cancellable);
+               op_data->cancellable = NULL;
        }
 
-       pending_ops = e_gdbus_async_op_keeper_get_pending_ops (E_GDBUS_ASYNC_OP_KEEPER (op_data->proxy));
-       if (pending_ops)
-               g_hash_table_remove (pending_ops, GUINT_TO_POINTER (op_data->opid));
+       if (!g_atomic_int_dec_and_test (&op_data->ref_count))
+               return;
+
        g_object_unref (op_data->proxy);
 
        switch (op_data->result_type) {
@@ -919,6 +930,7 @@ async_op_complete (AsyncOpData *op_data,
 
        g_return_if_fail (op_data != NULL);
 
+       g_atomic_int_inc (&op_data->ref_count);
        simple = g_simple_async_result_new (G_OBJECT (op_data->proxy), op_data->async_callback, op_data->async_user_data, op_data->async_source_tag);
        g_simple_async_result_set_op_res_gpointer (simple, op_data, (GDestroyNotify) async_op_data_free);
        if (error)
@@ -932,13 +944,43 @@ async_op_complete (AsyncOpData *op_data,
        g_object_unref (simple);
 }
 
+typedef struct _CancelData
+{
+       EGdbusAsyncOpKeeper *proxy;
+       guint opid;
+       AsyncOpData *op_data;
+} CancelData;
+
+static void
+cancel_data_free (gpointer ptr)
+{
+       CancelData *cd = ptr;
+
+       if (!cd)
+               return;
+
+       g_object_unref (cd->proxy);
+       g_free (cd);
+}
+
 static gboolean
 e_gdbus_op_cancelled_idle_cb (gpointer user_data)
 {
-       AsyncOpData *op_data = user_data;
+       CancelData *cd = user_data;
+       AsyncOpData *op_data;
+       GHashTable *pending_ops;
        GCancellable *cancellable;
        GError *error = NULL;
 
+       g_return_val_if_fail (cd != NULL, FALSE);
+
+       pending_ops = e_gdbus_async_op_keeper_get_pending_ops (cd->proxy);
+       if (pending_ops && !g_hash_table_lookup (pending_ops, GUINT_TO_POINTER (cd->opid))) {
+               /* got served already */
+               return FALSE;
+       }
+
+       op_data = cd->op_data;
        g_return_val_if_fail (op_data != NULL, FALSE);
 
        cancellable = op_data->cancellable;
@@ -961,12 +1003,19 @@ static void
 e_gdbus_op_cancelled_cb (GCancellable *cancellable,
                          AsyncOpData *op_data)
 {
+       CancelData *cd;
+
        g_return_if_fail (op_data != NULL);
        g_return_if_fail (op_data->cancellable == cancellable);
 
+       cd = g_new0 (CancelData, 1);
+       cd->proxy = g_object_ref (op_data->proxy);
+       cd->opid = op_data->opid;
+       cd->op_data = op_data;
+
        /* do this on idle, because this callback should be left
         * as soon as possible, with no sync calls being done */
-       op_data->cancel_idle_id = g_idle_add (e_gdbus_op_cancelled_idle_cb, op_data);
+       op_data->cancel_idle_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE, e_gdbus_op_cancelled_idle_cb, cd, cancel_data_free);
 }
 
 static void
@@ -1370,9 +1419,21 @@ e_gdbus_proxy_sync_ready_cb (GObject *proxy,
                              GAsyncResult *result,
                              gpointer user_data)
 {
-       SyncOpData *sync_data = user_data;
+       gint sync_opid = GPOINTER_TO_INT (user_data);
+       gchar *sync_opid_ident;
+       SyncOpData *sync_data;
+
+       sync_opid_ident = g_strdup_printf ("EGdbusTemplates-SyncOp-%d", sync_opid);
+       sync_data = g_object_get_data (proxy, sync_opid_ident);
+       g_free (sync_opid_ident);
+
+       if (!sync_data) {
+               /* already finished operation; it can happen when the operation is cancelled,
+                  but the result is already waiting in an idle queue.
+               */
+               return;
+       }
 
-       g_return_if_fail (sync_data != NULL);
        g_return_if_fail (sync_data->flag != NULL);
 
        switch (sync_data->out_type) {
@@ -1415,12 +1476,17 @@ e_gdbus_proxy_call_sync (GDBusProxy *proxy,
                          guint out_type,
                          gpointer out_value)
 {
+       static volatile gint sync_op_counter = 0;
+       gint sync_opid;
+       gchar *sync_opid_ident;
        SyncOpData sync_data = { 0 };
 
        g_return_val_if_fail (proxy != NULL, FALSE);
        g_return_val_if_fail (start_func != NULL, FALSE);
        g_return_val_if_fail (finish_func != NULL, FALSE);
 
+       g_object_ref (proxy);
+
        switch (out_type) {
        case E_GDBUS_TYPE_VOID:
                sync_data.finish_func.finish_void = finish_func;
@@ -1443,6 +1509,7 @@ e_gdbus_proxy_call_sync (GDBusProxy *proxy,
                break;
        default:
                g_warning ("%s: Unknown 'out' E_GDBUS_TYPE %x", G_STRFUNC, out_type);
+               g_object_unref (proxy);
                return FALSE;
        }
 
@@ -1450,30 +1517,37 @@ e_gdbus_proxy_call_sync (GDBusProxy *proxy,
        sync_data.error = error;
        sync_data.out_type = out_type;
 
+       sync_opid = g_atomic_int_add (&sync_op_counter, 1);
+       sync_opid_ident = g_strdup_printf ("EGdbusTemplates-SyncOp-%d", sync_opid);
+       g_object_set_data (G_OBJECT (proxy), sync_opid_ident, &sync_data);
+
        switch (in_type) {
        case E_GDBUS_TYPE_VOID: {
                EGdbusCallStartVoid start = start_func;
-               start (proxy, cancellable, e_gdbus_proxy_sync_ready_cb, &sync_data);
+               start (proxy, cancellable, e_gdbus_proxy_sync_ready_cb, GINT_TO_POINTER (sync_opid));
        } break;
        case E_GDBUS_TYPE_BOOLEAN: {
                EGdbusCallStartBoolean start = start_func;
-               start (proxy, * ((gboolean *) in_value), cancellable, e_gdbus_proxy_sync_ready_cb, &sync_data);
+               start (proxy, * ((gboolean *) in_value), cancellable, e_gdbus_proxy_sync_ready_cb, GINT_TO_POINTER (sync_opid));
        } break;
        case E_GDBUS_TYPE_STRING: {
                EGdbusCallStartString start = start_func;
-               start (proxy, (const gchar *) in_value, cancellable, e_gdbus_proxy_sync_ready_cb, &sync_data);
+               start (proxy, (const gchar *) in_value, cancellable, e_gdbus_proxy_sync_ready_cb, GINT_TO_POINTER (sync_opid));
        } break;
        case E_GDBUS_TYPE_STRV: {
                EGdbusCallStartStrv start = start_func;
-               start (proxy, (const gchar * const *) in_value, cancellable, e_gdbus_proxy_sync_ready_cb, &sync_data);
+               start (proxy, (const gchar * const *) in_value, cancellable, e_gdbus_proxy_sync_ready_cb, GINT_TO_POINTER (sync_opid));
        } break;
        case E_GDBUS_TYPE_UINT: {
                EGdbusCallStartUint start = start_func;
-               start (proxy, * ((guint *) in_value), cancellable, e_gdbus_proxy_sync_ready_cb, &sync_data);
+               start (proxy, * ((guint *) in_value), cancellable, e_gdbus_proxy_sync_ready_cb, GINT_TO_POINTER (sync_opid));
        } break;
        default:
                g_warning ("%s: Unknown 'in' E_GDBUS_TYPE %x", G_STRFUNC, in_type);
                e_flag_free (sync_data.flag);
+               g_object_set_data (G_OBJECT (proxy), sync_opid_ident, NULL);
+               g_free (sync_opid_ident);
+               g_object_unref (proxy);
                return FALSE;
        }
 
@@ -1502,8 +1576,12 @@ e_gdbus_proxy_call_sync (GDBusProxy *proxy,
                /* is called in a dedicated thread */
                e_flag_wait (sync_data.flag);
        }
+       g_object_set_data (G_OBJECT (proxy), sync_opid_ident, NULL);
        e_flag_free (sync_data.flag);
 
+       g_free (sync_opid_ident);
+       g_object_unref (proxy);
+
        return sync_data.finish_result;
 }