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

[Aleksander Morgado]

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.

-- 
Aleksander