[PATCH v2] mbim-proxy: avoid double-free of 'opening device info' struct
Aleksander Morgado
aleksander at aleksander.es
Thu Aug 3 12:34:47 UTC 2017
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
More information about the libmbim-devel
mailing list