[PATCH] mbim-proxy: avoid double-free of 'opening device info' struct
Ben Chan
benchan at chromium.org
Mon Jul 31 17:46:21 UTC 2017
On Mon, Jul 31, 2017 at 9:26 AM, Dan Williams <dcbw at redhat.com> wrote:
> On Sun, 2017-07-30 at 17:58 +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.
>
> LGTM, though I wonder if we shouldn't have a small new helper function
> that does the list remove and complete_and_free() and takes a GError*
> to pass along. Might make it clearer what operations need to happen
> when cleaning up.
>
lgtm. I was wondering the same thing. It'd be good to make sure an
OpeningDevice is removed from self->priv->opening_devices when it's
being complete_and_free().
> Dan
>
>> 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
>> _______________________________________________
>> 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