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

Dan Williams dcbw at redhat.com
Thu Aug 3 15:01:05 UTC 2017


On Thu, 2017-08-03 at 14:34 +0200, Aleksander Morgado 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'.

LGTM!

Dan

>     [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!
> 
> ---
>  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
> _______________________________________________
> libmbim-devel mailing list
> libmbim-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libmbim-devel


More information about the libmbim-devel mailing list