[PATCH] mbim-proxy: avoid double-free of 'opening device info' struct
Dan Williams
dcbw at redhat.com
Mon Jul 31 16:26:12 UTC 2017
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.
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