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

[Dan Williams <dcbw redhat com>]

On Thu, 2012-11-29 at 11:37 -0800, Ben Chan wrote:
> The crash happens to be a reuse of an already disposed MMSerialPort
> object. I'm now tracing the new/dispose paths.

Total aside; lucky you, T-Mobile has PCS HSPA in your location.  Please
share it with the rest of us.

Sincerely yours,
Dan

> 
> Ben
> 
> 
>     /* Don't read any input if the current command isn't done being
> sent yet */
>     info = g_queue_peek_nth (priv->queue, 0);
>    49f60:       6940            ldr     r0, [r0, #20]
> <- 0x151F60
>    49f62:       f7c9 ebf6       blx     13750 <_init+0x1254>
> 
> 
> ==20047== Invalid read of size 4
> ==20047==    at 0x151F60: ??? (in /usr/sbin/ModemManager)
> ==20047==  Address 0x14 is not stack'd, malloc'd or (recently) free'd
> ==20047== 
> ==20047== 
> ==20047== Process terminating with default action of signal 11
> (SIGSEGV)
> ==20047==  Access not within mapped region at address 0x14
> ==20047==    at 0x151F60: ??? (in /usr/sbin/ModemManager)
> ==20047==  If you believe this happened as a result of a stack
> ==20047==  overflow in your program's main thread (unlikely but
> ==20047==  possible), you can try to increase the size of the
> ==20047==  main thread stack using the --main-stacksize= flag.
> ==20047==  The main thread stack size used in this run was 8388608.
> 
> 
> (ModemManager:2858): GLib-GObject-CRITICAL **: g_object_ref: assertion
> `G_IS_OBJECT (object)' failed
> (ModemManager:2858): GLib-GObject-CRITICAL **: g_object_ref: assertion
> `G_IS_OBJECT (object)' failed
> (ModemManager:2858): GLib-GObject-CRITICAL **: g_object_unref:
> assertion `G_IS_OBJECT (object)' failed
> (ModemManager:2858): GLib-GObject-CRITICAL **: g_object_unref:
> assertion `G_IS_OBJECT (object)' failed
> (ModemManager:2858): GLib-GObject-WARNING **: gsignal.c:2576: instance
> `0x78624028' has no handler with id `148'
> (ModemManager:2858): GLib-GObject-WARNING **: invalid unclassed
> pointer in cast to `MMSerialPort'
> (ModemManager:2858): GLib-GObject-CRITICAL **:
> g_type_instance_get_private: assertion `instance != NULL &&
> instance->g_class != NULL' failed
> 
> On Thu, Nov 29, 2012 at 11:26 AM, Dan Williams <dcbw redhat com>
> wrote:
>         On Tue, 2012-11-27 at 13:19 -0800, Ben Chan wrote:
>         > 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.
>         
>         
>         Any way to get MM running with --debug in those logs?  Also,
>         in your
>         sources, does line 767 match up with:
>         
>             info = g_queue_peek_nth (priv->queue, 0);
>         
>         or some other line?
>         
>         One other thing to do, put an
>         
>         mm_info ("(%s): disposing", mm_port_get_device (MM_PORT
>         (self)));
>         
>         into dispose() and lets see when it gets disposed and if
>         anything tries
>         to open it during/after dispose.  Drop another of these into
>         mm_serial_port_new().
>         
>         Dan
>         
>         >
>         > 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
>         >
>         >
>         
>         > _______________________________________________
>         > networkmanager-list mailing list
>         > networkmanager-list gnome org
>         > https://mail.gnome.org/mailman/listinfo/networkmanager-list
>         
>         
> 
>