[PATCH] serial-port: fail open/reopen after a serial port is disposed

[Aleksander Morgado]

On 17/12/13 22:48, Ben Chan wrote:
> After a MMSerialPort object is disposed and closed, it may be opened again.
> That could potentially lead to a crash if data_available is called after
> the MMSerialPort object is finalized. This patch prevents such a
> scenario by failing mm_serial_port_open / mm_serial_port_reopen on an
> already disposed MMSerialPort object.
> ---
> Dan / Aleksander,
> 
> The code shouldn't really call open/reopen on an already disposed MMSerialPort,
> I wonder if we should use g_assert (!priv->disposed) instead of reporting an
> error. That would allow us to pin point the source of the problem more easily.
> How do you think?
> 

In our current setup we do allow disposed objects which are still not
yet finished. In other words, we allow to have an object in which
dispose() was called (e.g. g_object_run_dispose()) and still have valid
references around (i.e. finalize() wasn't called yet). But, from what I
can recall, this was only applicable to 'modem' objects, so that we
could cleanup circular references between objects.

I'm not sure that we allow in any way non-finalized-but-disposed ports,
unless it's a reference bug (i.e. we're using already unref-ed objects).

After thinking about it further, I believe that the real problem here
could be that the port got 'forced-close' (modem unplugged and we get
HUP in the data_available() callback, which triggers forced closing) and
after that we try to open it again (so the reference is still valid, no
one called dispose or finalize). When the port disappears we do clean
the GObjects from the MMBaseModem, *but* there may be some other port
reference around (e.g. in an async method context we may have a
reference to the primary port). An easy fix for this would be to return
an error in open() or reopen() if priv->forced_close is set.

Does this make sense?


> Thanks,
> Ben
> 
>  src/mm-serial-port.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/mm-serial-port.c b/src/mm-serial-port.c
> index 545e707..d3361d2 100644
> --- a/src/mm-serial-port.c
> +++ b/src/mm-serial-port.c
> @@ -70,6 +70,7 @@ static guint signals[LAST_SIGNAL] = { 0 };
>  
>  typedef struct {
>      guint32 open_count;
> +    gboolean disposed;
>      gboolean forced_close;
>      int fd;
>      GHashTable *reply_cache;
> @@ -875,6 +876,15 @@ mm_serial_port_open (MMSerialPort *self, GError **error)
>      priv = MM_SERIAL_PORT_GET_PRIVATE (self);
>      device = mm_port_get_device (MM_PORT (self));
>  
> +    if (priv->disposed) {
> +        g_set_error (error,
> +                     MM_SERIAL_ERROR,
> +                     MM_SERIAL_ERROR_OPEN_FAILED,
> +                     "Could not open serial device %s: it is being disposed",
> +                     device);
> +        return FALSE;
> +    }
> +
>      if (priv->reopen_id) {
>          g_set_error (error,
>                       MM_SERIAL_ERROR,
> @@ -1293,6 +1303,17 @@ mm_serial_port_reopen (MMSerialPort *self,
>      g_return_val_if_fail (MM_IS_SERIAL_PORT (self), FALSE);
>      priv = MM_SERIAL_PORT_GET_PRIVATE (self);
>  
> +    if (priv->disposed) {
> +        GError *error;
> +
> +        error = g_error_new_literal (MM_CORE_ERROR,
> +                                     MM_CORE_ERROR_FAILED,
> +                                     "Serial port is being disposed.");
> +        callback (self, error, user_data);
> +        g_error_free (error);
> +        return FALSE;
> +    }
> +
>      if (priv->reopen_id > 0) {
>          GError *error;
>  
> @@ -1691,6 +1712,9 @@ dispose (GObject *object)
>      serial_port_reopen_cancel (MM_SERIAL_PORT (object));
>      mm_serial_port_flash_cancel (MM_SERIAL_PORT (object));
>  
> +    /* Mark the serial port as disposed to fail subsequent attempts to open/reopen the port. */
> +    priv->disposed = TRUE;
> +
>      G_OBJECT_CLASS (mm_serial_port_parent_class)->dispose (object);
>  }
>  
> 


-- 
Aleksander