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

Aleksander Morgado aleksander at aleksander.es
Thu Aug 3 15:13:15 UTC 2017


On Thu, Aug 3, 2017 at 2:34 PM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> 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,
>
> This updated patch introduces a helper complete_opening_device() that takes care of both removing the info from the list and freeing it. I believe it does look much cleaner indeed.
>
> What do you think?
>
> Cheers!
>

Pushed to git master, mbim-1-14, mbim-1-12 and mbim-1-10.

> ---
>  src/libmbim-glib/mbim-proxy.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/src/libmbim-glib/mbim-proxy.c b/src/libmbim-glib/mbim-proxy.c
> index fdbc7d9..d59a4de 100644
> --- a/src/libmbim-glib/mbim-proxy.c
> +++ b/src/libmbim-glib/mbim-proxy.c
> @@ -461,6 +461,21 @@ peek_opening_device_info (MbimProxy  *self,
>  }
>
>  static void
> +complete_opening_device (MbimProxy    *self,
> +                         MbimDevice   *device,
> +                         const GError *error)
> +{
> +    OpeningDevice *info;
> +
> +    info = peek_opening_device_info (self, device);
> +    if (!info)
> +        return;
> +
> +    self->priv->opening_devices = g_list_remove (self->priv->opening_devices, info);
> +    opening_device_complete_and_free (info, error);
> +}
> +
> +static void
>  cancel_opening_device (MbimProxy  *self,
>                         MbimDevice *device)
>  {
> @@ -472,7 +487,7 @@ cancel_opening_device (MbimProxy  *self,
>          return;
>
>      error = g_error_new (MBIM_CORE_ERROR, MBIM_CORE_ERROR_ABORTED, "Device is gone");
> -    opening_device_complete_and_free (info, error);
> +    complete_opening_device (self, device, error);
>      g_error_free (error);
>  }
>
> @@ -482,18 +497,11 @@ device_open_ready (MbimDevice   *device,
>                     MbimProxy    *self)
>  {
>      GError *error = NULL;
> -    OpeningDevice *info;
>
>      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);
> +    complete_opening_device (self, device, error);
>
>      if (error) {
>          /* Fully untrack the device as it wasn't correctly open */
> --
> 2.13.1



-- 
Aleksander
https://aleksander.es


More information about the libmbim-devel mailing list