Hi again,
On Tue, Oct 16, 2012 at 7:34 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Szymon,
>
> On Tue, Oct 16, 2012 at 2:37 PM, Szymon Janc <szymon.janc@xxxxxxxxx> wrote:
>> This fix avdtp session reference counting. avdtp_unref has a special
>> case when ref==1 and due to nested calls session could be free in
>> consecutive call resulting in accesing already freed memory.
>>
>> Instead of having extra free lock keep extra local reference. This also
>> make avdtp_unref easier to undertand as there is no double ref
>> decrement inside one unref call.
>>
>> avdtp_unref will set session state to disconnected when reference count
>> drops to 1 so there is no need to explicite set it in connection_lost.
>>
>> bluetoothd[29474]: audio/avdtp.c:avdtp_ref() 0x555555856aa0: ref=2
>> bluetoothd[29474]: audio/source.c:source_connect() stream creation in progress
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 31 bytes from management socket
>> bluetoothd[29474]: src/mgmt.c:mgmt_device_connected() hci0 device 84:00:D2:DB:F7:8C connected eir_len 12
>> bluetoothd[29474]: src/adapter.c:adapter_get_device() 84:00:D2:DB:F7:8C
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 14 bytes from management socket
>> bluetoothd[29474]: src/mgmt.c:mgmt_auth_failed() hci0 auth failed status 6
>> bluetoothd[29474]: src/device.c:device_bonding_complete() bonding (nil) status 0x06
>> bluetoothd[29474]: connect error: Invalid exchange (52)
>> bluetoothd[29474]: audio/avdtp.c:connection_lost() Disconnected from 84:00:D2:DB:F7:8C
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=1
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=0
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> avdtp_unref (session=0x555555856aa0) at audio/avdtp.c:1239
>> 1239 server->sessions = g_slist_remove(server->sessions, session);
>>
>> ---
>> audio/avdtp.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/audio/avdtp.c b/audio/avdtp.c
>> index bd91cb6..4109be9 100644
>> --- a/audio/avdtp.c
>> +++ b/audio/avdtp.c
>> @@ -388,7 +388,6 @@ struct avdtp_stream {
>>
>> struct avdtp {
>> int ref;
>> - int free_lock;
>>
>> uint16_t version;
>>
>> @@ -1158,14 +1157,14 @@ static void connection_lost(struct avdtp *session, int err)
>> if (err != EACCES)
>> avdtp_cancel_authorization(session);
>>
>> - session->free_lock = 1;
>> + avdtp_ref(session);
>>
>> finalize_discovery(session, err);
>>
>> g_slist_foreach(session->streams, (GFunc) release_stream, session);
>> session->streams = NULL;
>>
>> - session->free_lock = 0;
>> + avdtp_unref(session);
>>
>> if (session->io) {
>> g_io_channel_shutdown(session->io, FALSE, NULL);
>> @@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
>> session->io = NULL;
>> }
>>
>> - avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
>
> This is actually the one that we need in case we got disconnect by
> remote after discovery completes. avctp.c has a similar structure to
> handler such states, but without the refcounting all is much simpler.
>
>> if (session->io_id) {
>> g_source_remove(session->io_id);
>> session->io_id = 0;
>> @@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
>>
>> if (session->io)
>> set_disconnect_timer(session);
>> - else if (!session->free_lock) /* Drop the local ref if we
>> - aren't connected */
>> - session->ref--;
>> }
>>
>> if (session->ref > 0)
>> --
>> 1.7.9.5
>
> We should probably get rid of the free_lock as well.
I just remembered the reason we have this strange refcount 1, it was
to keep the session up while the unix client/alsa client reconnects. I
think we can drop this altogether now that we don't support such bogus
clients, this also means dropping dc_timer and which should fix the
refcount for good.
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
