[PATCH v2] mbim-proxy: avoid double-free of 'opening device info' struct
Ben Chan
benchan at chromium.org
Thu Aug 3 15:06:59 UTC 2017
lgtm
On Aug 3, 2017 5:34 AM, "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!
>
> ---
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libmbim-devel/attachments/20170803/28762d09/attachment-0001.html>
More information about the libmbim-devel
mailing list