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

[Ben Chan]

On Wed, Dec 18, 2013 at 10:13 AM, Aleksander Morgado
<aleksander at lanedo.com>wrote:

> On 18/12/13 18:35, 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?
> >> >
> > If some async method or timeout context still holds a reference to the
> port,
> > finalize shouldn't have been invoked, right? If finalize is invoked,
> > that seems like
> > another reference bug?
> >
>
> Exactly, yes to both.
>
> > Related to the HUP case, is the port always removed by the kernel?
> > I sometimes observed that ModemManager was able to communicate with the
> > modem after restarting. so I wonder if the 'port hangup' scenario is
> temporary.
>
> If the port HUP happens, we force-close and we actually just assume
> we'll get a udev event to get the port removed. Not sure if we can
> consider those as temporary failures; as the port will get re-plugged
> and re-notified by udev, which will trigger a full new port probing setup.
>
>
I examined the references of the port and they all seemed ok. The actual
issue may be the following:

1. data_available() calls mm_serial_port_close_force() to force close a
port upon receiving G_IO_HUP. priv->forced_close is set to TRUE.
2. The connection/disconnection sequence still holds a reference to the
port and eventually triggers the port to be re-opened. However,
mm_serial_port_open() doesn't reset priv->forced_close to FALSE.
3. When the port is no longer referenced, dispose() calls
mm_serial_port_close_force(), which does nothing because priv->forced_close
is TRUE.
4. priv->watch_id is still active and eventually calls data_available()
after the port is finalized. data_available() accesses priv->queue and
crashes.

We can either (1) reset priv->forced_close to FALSE in mm_serial_port_open,
or (2) fail open/repoen if priv->forced_close is TRUE. (1) assumes it's ok
to repoen a force-closed port while (2) doesn't.  Dan / Aleksander, which
semantic do you prefer?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20140107/d3f560a2/attachment.html>