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

[Aleksander Morgado]

On 08/01/14 00:44, 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?
> 

Forced-close is done only when the port gets removed (i.e. we get a
HUP); so I'd suggest to go for option #2 (i.e. error if trying to reopen
a forced-closed port).

-- 
Aleksander