<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 18, 2013 at 10:13 AM, Aleksander Morgado <span dir="ltr"><<a href="mailto:aleksander@lanedo.com" target="_blank">aleksander@lanedo.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On 18/12/13 18:35, Ben Chan wrote:<br>


>>> After a MMSerialPort object is disposed and closed, it may be opened again.<br>
>>> > > That could potentially lead to a crash if data_available is called after<br>
>>> > > the MMSerialPort object is finalized. This patch prevents such a<br>
>>> > > scenario by failing mm_serial_port_open / mm_serial_port_reopen on an<br>
>>> > > already disposed MMSerialPort object.<br>
>>> > > ---<br>
>>> > > Dan / Aleksander,<br>
>>> > ><br>
>>> > > The code shouldn't really call open/reopen on an already disposed MMSerialPort,<br>
>>> > > I wonder if we should use g_assert (!priv->disposed) instead of reporting an<br>
>>> > > error. That would allow us to pin point the source of the problem more easily.<br>
>>> > > How do you think?<br>
>>> > ><br>
>> ><br>
>> > In our current setup we do allow disposed objects which are still not<br>
>> > yet finished. In other words, we allow to have an object in which<br>
>> > dispose() was called (e.g. g_object_run_dispose()) and still have valid<br>
>> > references around (i.e. finalize() wasn't called yet). But, from what I<br>
>> > can recall, this was only applicable to 'modem' objects, so that we<br>
>> > could cleanup circular references between objects.<br>
>> ><br>
>> > I'm not sure that we allow in any way non-finalized-but-disposed ports,<br>
>> > unless it's a reference bug (i.e. we're using already unref-ed objects).<br>
>> ><br>
>> > After thinking about it further, I believe that the real problem here<br>
>> > could be that the port got 'forced-close' (modem unplugged and we get<br>
>> > HUP in the data_available() callback, which triggers forced closing) and<br>
>> > after that we try to open it again (so the reference is still valid, no<br>
>> > one called dispose or finalize). When the port disappears we do clean<br>
>> > the GObjects from the MMBaseModem, *but* there may be some other port<br>
>> > reference around (e.g. in an async method context we may have a<br>
>> > reference to the primary port). An easy fix for this would be to return<br>
>> > an error in open() or reopen() if priv->forced_close is set.<br>
>> ><br>
>> > Does this make sense?<br>
>> ><br>
> If some async method or timeout context still holds a reference to the port,<br>
> finalize shouldn't have been invoked, right? If finalize is invoked,<br>
> that seems like<br>
> another reference bug?<br>
><br>
<br>
</div></div>Exactly, yes to both.<br>
<div class="im"><br>
> Related to the HUP case, is the port always removed by the kernel?<br>
> I sometimes observed that ModemManager was able to communicate with the<br>
> modem after restarting. so I wonder if the 'port hangup' scenario is temporary.<br>
<br>
</div>If the port HUP happens, we force-close and we actually just assume<br>
we'll get a udev event to get the port removed. Not sure if we can<br>
consider those as temporary failures; as the port will get re-plugged<br>
and re-notified by udev, which will trigger a full new port probing setup.<br>
<span class=""><font color="#888888"><br></font></span></blockquote><div><br></div><div>I examined the references of the port and they all seemed ok. The actual issue may be the following:</div><div><br></div><div>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.</div>

<div>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.</div><div>

3. When the port is no longer referenced, dispose() calls mm_serial_port_close_force(), which does nothing because priv->forced_close is TRUE.</div><div>4. priv->watch_id is still active and eventually calls data_available() after the port is finalized. data_available() accesses priv->queue and crashes.</div>

<div><br></div><div>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?</div>

<div> </div></div></div></div>