[PATCH] libmbim-glib: mbim-device: Fix adding multiple timeouts for a transaction

[Aleksander Morgado]

Hey,

Good catch indeed, see a minor comment below.

On Tue, Feb 4, 2014 at 4:25 AM, Greg Suarez <gpsuarez2512 at gmail.com> wrote:
> When receiving multiple fragments device_store_transaction() is called
> for each fragment received thus adding a timeout callback for each
> fragment.  A timeout callback is only removed when all fragments are
> received thus leaving dangling timeout callbacks.  This leads to a crash
> when the MbimDevice is finalized.
>
> This patch checks if the transaction has already registered a timeout
> callback before adding a new one.
>
> Signed-off-by: Greg Suarez <gpsuarez2512 at gmail.com>
> ---
>  src/libmbim-glib/mbim-device.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/src/libmbim-glib/mbim-device.c b/src/libmbim-glib/mbim-device.c
> index d8c6f81..ef47b7e 100644
> --- a/src/libmbim-glib/mbim-device.c
> +++ b/src/libmbim-glib/mbim-device.c
> @@ -267,21 +267,24 @@ device_store_transaction (MbimDevice       *self,
>      tr->wait_ctx->transaction_id = tr->transaction_id;
>      tr->wait_ctx->type = type;
>
> -    tr->timeout_id = g_timeout_add (timeout_ms,
> -                                    (GSourceFunc)transaction_timed_out,
> -                                    tr->wait_ctx);
> -
> -    if (tr->cancellable) {
> -        tr->cancellable_id = g_cancellable_connect (tr->cancellable,
> -                                                    (GCallback)transaction_cancelled,
> -                                                    tr->wait_ctx,
> -                                                    NULL);
> -        if (!tr->cancellable_id) {
> -            g_set_error_literal (error,
> -                                 MBIM_CORE_ERROR,
> -                                 MBIM_CORE_ERROR_ABORTED,
> -                                 "Request is already cancelled");
> -            return FALSE;
> +    /* don't add timeout if one already exists */
> +    if (!tr->timeout_id) {
> +        tr->timeout_id = g_timeout_add (timeout_ms,
> +                                        (GSourceFunc)transaction_timed_out,
> +                                        tr->wait_ctx);
> +

How about moving the if (tr->cancellable) change  out of the new if
(!tr->timeout_id) you added, and also extend it so that it checks if
(tr->cancellable && !tr->cancellable_id) ?

Just to separate the logic of the timeout with that of the cancellable.

> +        if (tr->cancellable) {
> +            tr->cancellable_id = g_cancellable_connect (tr->cancellable,
> +                                                        (GCallback)transaction_cancelled,
> +                                                        tr->wait_ctx,
> +                                                        NULL);
> +            if (!tr->cancellable_id) {
> +                g_set_error_literal (error,
> +                                     MBIM_CORE_ERROR,
> +                                     MBIM_CORE_ERROR_ABORTED,
> +                                     "Request is already cancelled");
> +                return FALSE;
> +            }
>          }
>      }
>
> --
> 1.8.5.3
>



-- 
Aleksander