[PATCH] mbim-proxy: avoid double-free of 'opening device info' struct

[Aleksander Morgado]

When the device is gone while an open() operation is ongoing, we're
completing and freeing the 'opening device info' struct, but we didn't
remove it from the private info, so when device_open_ready() happened,
we were trying to complete and free it again.

Avoid this issue by making sure no already-freed structs are kept in
the private info, and also allowing device_open_ready() to get called
without a valid pending 'opening device info'.

    [30 Jul 2017, 15:24:33] [Debug] [/dev/cdc-wdm0] Sent message (translated)...
    <<<<<< Header:
    <<<<<<   length      = 16
    <<<<<<   type        = open (0x00000001)
    <<<<<<   transaction = 1
    <<<<<< Contents:
    <<<<<<   max_control_transfer = 4096

    [30 Jul 2017, 15:24:38] [Debug] [/dev/cdc-wdm0] Sent message...
    <<<<<< RAW:
    <<<<<<   length = 16
    <<<<<<   data   = 01:00:00:00:10:00:00:00:02:00:00:00:00:10:00:00

    [30 Jul 2017, 15:24:38] [Debug] [/dev/cdc-wdm0] Sent message (translated)...
    <<<<<< Header:
    <<<<<<   length      = 16
    <<<<<<   type        = open (0x00000001)
    <<<<<<   transaction = 2
    <<<<<< Contents:
    <<<<<<   max_control_transfer = 4096

    [30 Jul 2017, 15:24:42] [Debug] [/dev/cdc-wdm0] unexpected port hangup!
    [30 Jul 2017, 15:24:42] [Debug] Client (7) connection closed...
    [30 Jul 2017, 15:24:42] -Warning ** error opening device: Device is gone
    [30 Jul 2017, 15:24:43] [Debug] open operation timed out: closed
    ==24404== Invalid read of size 8
    ==24404==    at 0x4E4A673: peek_opening_device_info (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x4E4BD7B: device_open_ready (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x50CAF61: g_simple_async_result_complete (in /usr/lib/libgio-2.0.so.0.3707.0)
    ==24404==    by 0x50CB028: complete_in_idle_cb (in /usr/lib/libgio-2.0.so.0.3707.0)
    ==24404==    by 0x5602EEA: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x5603207: g_main_context_iterate.isra.13 (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x56035D1: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x4010F8: main (in /usr/lib/mbim-proxy)
    ==24404==  Address 0x6d3bfe0 is 0 bytes inside a block of size 16 free'd
    ==24404==    at 0x4C2A0C0: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==24404==    by 0x4E4AC21: untrack_device (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x5395392: _g_closure_invoke_va (in /usr/lib/libgobject-2.0.so.0.3707.0)
    ==24404==    by 0x53A805D: g_signal_emit_valist (in /usr/lib/libgobject-2.0.so.0.3707.0)
    ==24404==    by 0x53A8A51: g_signal_emit (in /usr/lib/libgobject-2.0.so.0.3707.0)
    ==24404==    by 0x4E49424: data_available (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x5602EEA: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x5603207: g_main_context_iterate.isra.13 (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x56035D1: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x4010F8: main (in /usr/lib/mbim-proxy)
    ==24404==  Block was alloc'd at
    ==24404==    at 0x4C2B3D0: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==24404==    by 0x5607B00: g_malloc (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x5617E12: g_slice_alloc (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x5618385: g_slice_alloc0 (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x4E4B1A9: internal_open (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x4E4BAC5: device_new_ready (in /usr/lib/libmbim-glib.so.4.2.0)
    ==24404==    by 0x50CAF61: g_simple_async_result_complete (in /usr/lib/libgio-2.0.so.0.3707.0)
    ==24404==    by 0x50CB028: complete_in_idle_cb (in /usr/lib/libgio-2.0.so.0.3707.0)
    ==24404==    by 0x5602EEA: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x5603207: g_main_context_iterate.isra.13 (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x56035D1: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.3707.0)
    ==24404==    by 0x4010F8: main (in /usr/lib/mbim-proxy)
    ==24404==
---

Hey Ben, Dan & everyone,

Wouldn't mind some extra pair of eyes reviewing this patch...

Cheers!

---
 src/libmbim-glib/mbim-proxy.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/libmbim-glib/mbim-proxy.c b/src/libmbim-glib/mbim-proxy.c
index fdbc7d9..1931ea7 100644
--- a/src/libmbim-glib/mbim-proxy.c
+++ b/src/libmbim-glib/mbim-proxy.c
@@ -471,6 +471,9 @@ cancel_opening_device (MbimProxy  *self,
     if (!info)
         return;

+    /* Remove opening device info */
+    self->priv->opening_devices = g_list_remove (self->priv->opening_devices, info);
+    /* Complete all pending open actions */
     error = g_error_new (MBIM_CORE_ERROR, MBIM_CORE_ERROR_ABORTED, "Device is gone");
     opening_device_complete_and_free (info, error);
     g_error_free (error);
@@ -487,13 +490,12 @@ device_open_ready (MbimDevice   *device,
     mbim_device_open_finish (device, res, &error);

     info = peek_opening_device_info (self, device);
-    g_assert (info != NULL);
-
-    /* Remove opening device info */
-    self->priv->opening_devices = g_list_remove (self->priv->opening_devices, info);
-
-    /* Complete all pending open actions */
-    opening_device_complete_and_free (info, error);
+    if (info) {
+        /* Remove opening device info */
+        self->priv->opening_devices = g_list_remove (self->priv->opening_devices, info);
+        /* Complete all pending open actions */
+        opening_device_complete_and_free (info, error);
+    }

     if (error) {
         /* Fully untrack the device as it wasn't correctly open */
--
2.13.1