Re: [MM] [PATCH v2] serial-port: avoid opening a serial port that has been disposed

[Ben Chan <benchan chromium org>]

Yes, it's related to the data_available (mm-serial-port.c:767) crash (crosbug.com/35391).  I'm running suspend/resume stress test with ModemManager under valgrind.

Thanks,

Ben

On Tue, Nov 27, 2012 at 1:13 PM, Aleksander Morgado <aleksander lanedo com> wrote:

On 11/27/2012 09:39 PM, Ben Chan wrote:
> ---
>  src/mm-serial-port.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)

Don't think it's the correct approach.

I don't think we ever run g_object_run_dispose() ourselves for port
objects, and that means that whenever we got the port object disposed it
was because it was the last valid reference.

And that means that you won't be able to read priv->disposed afterwards
as that would mean reading already freed memory.

There clearly is a missing reference around, or a timeout or other
source which has the port as user_data and is not being properly cleaned
up (e.g. removing the timeout or event source when the port is
disposed). Running it under valgrind should confirm this. Is this
related to the data_available() crashes during suspend/resume?

>
> diff --git a/src/mm-serial-port.c b/src/mm-serial-port.c
> index 0a8820d..a33c745 100644
> --- a/src/mm-serial-port.c
> +++ b/src/mm-serial-port.c
> @@ -69,6 +69,7 @@ static guint signals[LAST_SIGNAL] = { 0 };
>
>  typedef struct {
>      guint32 open_count;
> +    gboolean disposed;
>      gboolean forced_close;
>      int fd;
>      GHashTable *reply_cache;
> @@ -849,6 +850,12 @@ mm_serial_port_open (MMSerialPort *self, GError **error)
>
>      device = mm_port_get_device (MM_PORT (self));
>
> +    /* If the MMSerialPort object has been disposed, just return an error. */
> +    if (priv->disposed) {
> +        mm_info ("(%s) skipped opening serial port that has been disposed", device);
> +        return FALSE;
> +    }
> +
>      if (priv->open_count) {
>          /* Already open */
>          goto success;
> @@ -1537,6 +1544,9 @@ dispose (GObject *object)
>  {
>      MMSerialPortPrivate *priv = MM_SERIAL_PORT_GET_PRIVATE (object);
>
> +    /* Mark the MMSerialPort object as disposed to prevent it from being re-opened. */
> +    priv->disposed = TRUE;
> +
>      if (priv->timeout_id) {
>          g_source_remove (priv->timeout_id);
>          priv->timeout_id = 0;
>


--

Aleksander