Re: [PATCH] broadband-modem-mbim: explicitly remove notification handler on unref
On Fri, Aug 11, 2017 at 5:04 PM, Aleksander Morgado
wrote:
> When we remove the last object reference, make sure the notification
> handler is also removed, or we may end up using an already freed
> object.
>
> https://retrace.fedoraproject.org/faf/reports/1815001/
Pushed to git master, and backported to mm-1-6 and mm-1-4
> ---
> src/mm-broadband-modem-mbim.c | 61
> +++
> 1 file changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index 815f5455..8a722b11 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -2215,24 +2215,12 @@ device_notification_cb (MbimDevice *device,
> }
> }
>
> -static gboolean
> -common_setup_cleanup_unsolicited_events_finish (MMBroadbandModemMbim *self,
> -GAsyncResult *res,
> -GError **error)
> -{
> -return g_task_propagate_boolean (G_TASK (res), error);
> -}
> -
> static void
> -common_setup_cleanup_unsolicited_events (MMBroadbandModemMbim *self,
> - gboolean setup,
> - GAsyncReadyCallback callback,
> - gpointer user_data)
> +common_setup_cleanup_unsolicited_events_sync (MMBroadbandModemMbim *self,
> + MbimDevice *device,
> + gboolean setup)
> {
> -MbimDevice *device;
> -GTask *task;
> -
> -if (!peek_device (self, &device, callback, user_data))
> +if (!device)
> return;
>
> mm_dbg ("Supported notifications: signal (%s), registration (%s), sms
> (%s), connect (%s), subscriber (%s), packet (%s)",
> @@ -2260,6 +2248,29 @@ common_setup_cleanup_unsolicited_events
> (MMBroadbandModemMbim *self,
> self->priv->notification_id = 0;
> }
> }
> +}
> +
> +static gboolean
> +common_setup_cleanup_unsolicited_events_finish (MMBroadbandModemMbim *self,
> +GAsyncResult *res,
> +GError **error)
> +{
> +return g_task_propagate_boolean (G_TASK (res), error);
> +}
> +
> +static void
> +common_setup_cleanup_unsolicited_events (MMBroadbandModemMbim *self,
> + gboolean setup,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> +GTask *task;
> +MbimDevice *device;
> +
> +if (!peek_device (self, &device, callback, user_data))
> +return;
> +
> +common_setup_cleanup_unsolicited_events_sync (self, device, setup);
>
> task = g_task_new (self, NULL, callback, user_data);
> g_task_return_boolean (task, TRUE);
> @@ -3174,20 +3185,24 @@ mm_broadband_modem_mbim_init (MMBroadbandModemMbim
> *self)
> static void
> finalize (GObject *object)
> {
> -MMPortMbim *mbim;
> MMBroadbandModemMbim *self = MM_BROADBAND_MODEM_MBIM (object);
> +MMPortMbim *mbim;
> +
> +mbim = mm_base_modem_peek_port_mbim (MM_BASE_MODEM (self));
> +if (mbim) {
> +/* Explicitly remove notification handler */
> +self->priv->setup_flags = PROCESS_NOTIFICATION_FLAG_NONE;
> +common_setup_cleanup_unsolicited_events_sync (self,
> mm_port_mbim_peek_device (mbim), FALSE);
> +/* If we did open the MBIM port during initialization, close it now
> */
> +if (mm_port_mbim_is_open (mbim))
> +mm_port_mbim_close (mbim, NULL, NULL);
> +}
>
> g_free (self->priv->caps_device_id);
> g_free (self->priv->caps_firmware_info);
> g_free (self->priv->current_operator_id);
> g_free (self->priv->current_operator_name);
>
> -mbim = mm_base_modem_peek_port_mbim (MM_BASE_MODEM (self));
> -/* If we did open the MBIM port during initialization, close it now */
> -if (mbim && mm_port_mbim_is_open (mbim)) {
> -mm_port_mbim_close (mbim, NULL, NULL);
> -}
> -
> G_OBJECT_CLASS (mm_broadband_modem_mbim_parent_class)->finalize (object);
> }
>
> --
> 2.13.1
>
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH] broadband-modem-mbim: explicitly remove notification handler on unref
>> When we remove the last object reference, make sure the notification >> handler is also removed, or we may end up using an already freed >> object. >> >> https://retrace.fedoraproject.org/faf/reports/1815001/ > > Good catch; any time we connect a signal somewhere in an object, we > should probably have an explicit disconnect/clear of that handler in > the finalize/dispose path just to be sure. > > Which brings up the question, should that kind of thing go in dispose? > Honestly I don't think it makes much of a difference, except that if > it's in finalize we might get the handler triggered during dispose when > half the object is cleaned up already. > I'm not sure there's any convention regarding one or the other; truth be told. I've usually used dispose() only to cleanup GObjects, as g_object_run_dispose() may be used to cleanup circular references, and finalize() for everything else. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH] broadband-modem-mbim: explicitly remove notification handler on unref
On Fri, 2017-08-11 at 17:04 +0200, Aleksander Morgado wrote:
> When we remove the last object reference, make sure the notification
> handler is also removed, or we may end up using an already freed
> object.
>
> https://retrace.fedoraproject.org/faf/reports/1815001/
Good catch; any time we connect a signal somewhere in an object, we
should probably have an explicit disconnect/clear of that handler in
the finalize/dispose path just to be sure.
Which brings up the question, should that kind of thing go in dispose?
Honestly I don't think it makes much of a difference, except that if
it's in finalize we might get the handler triggered during dispose when
half the object is cleaned up already.
Dan
> ---
> src/mm-broadband-modem-mbim.c | 61 +++
>
> 1 file changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-
> mbim.c
> index 815f5455..8a722b11 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -2215,24 +2215,12 @@ device_notification_cb (MbimDevice *device,
> }
> }
>
> -static gboolean
> -common_setup_cleanup_unsolicited_events_finish (MMBroadbandModemMbim
> *self,
> -GAsyncResult *res,
> -GError **error)
> -{
> -return g_task_propagate_boolean (G_TASK (res), error);
> -}
> -
> static void
> -common_setup_cleanup_unsolicited_events (MMBroadbandModemMbim *self,
> - gboolean setup,
> - GAsyncReadyCallback
> callback,
> - gpointer user_data)
> +common_setup_cleanup_unsolicited_events_sync (MMBroadbandModemMbim
> *self,
> + MbimDevice *
> device,
> + gboolean
> setup)
> {
> -MbimDevice *device;
> -GTask *task;
> -
> -if (!peek_device (self, &device, callback, user_data))
> +if (!device)
> return;
>
> mm_dbg ("Supported notifications: signal (%s), registration
> (%s), sms (%s), connect (%s), subscriber (%s), packet (%s)",
> @@ -2260,6 +2248,29 @@ common_setup_cleanup_unsolicited_events
> (MMBroadbandModemMbim *self,
> self->priv->notification_id = 0;
> }
> }
> +}
> +
> +static gboolean
> +common_setup_cleanup_unsolicited_events_finish
> (MMBroadbandModemMbim *self,
> +GAsyncResult
> *res,
> +GError
> **error)
> +{
> +return g_task_propagate_boolean (G_TASK (res), error);
> +}
> +
> +static void
> +common_setup_cleanup_unsolicited_events (MMBroadbandModemMbim *self,
> + gboolean setup
> ,
> + GAsyncReadyCallback callb
> ack,
> + gpointer user_
> data)
> +{
> +GTask *task;
> +MbimDevice *device;
> +
> +if (!peek_device (self, &device, callback, user_data))
> +return;
> +
> +common_setup_cleanup_unsolicited_events_sync (self, device,
> setup);
>
> task = g_task_new (self, NULL, callback, user_data);
> g_task_return_boolean (task, TRUE);
> @@ -3174,20 +3185,24 @@ mm_broadband_modem_mbim_init
> (MMBroadbandModemMbim *self)
> static void
> finalize (GObject *object)
> {
> -MMPortMbim *mbim;
> MMBroadbandModemMbim *self = MM_BROADBAND_MODEM_MBIM (object);
> +MMPortMbim *mbim;
> +
> +mbim = mm_base_modem_peek_port_mbim (MM_BASE_MODEM (self));
> +if (mbim) {
> +/* Explicitly remove notification handler */
> +self->priv->setup_flags = PROCESS_NOTIFICATION_FLAG_NONE;
> +common_setup_cleanup_unsolicited_events_sync (self,
> mm_port_mbim_peek_device (mbim), FALSE);
> +/* If we did open the MBIM port during initialization, close
> it now */
> +if (mm_port_mbim_is_open (mbim))
> +mm_port_mbim_close (mbim, NULL, NULL);
> +}
>
> g_free (self->priv->caps_device_id);
> g_free (self->priv->caps_firmware_info);
> g_free (self->priv->current_operator_id);
> g_free (self->priv->current_operator_name);
>
> -mbim = mm_base_modem_peek_port_mbim (MM_BASE_MODEM (self));
> -/* If we did open the MBIM port during initialization, close it
> now */
> -if (mbim && mm_port_mbim_is_open (mbim)) {
> -mm_port_mbim_close (mbim, NULL, NULL);
> -}
> -
> G_OBJECT_CLASS (mm_broadband_modem_mbim_parent_class)->finalize
> (object);
> }
>
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
