On 20.07.2017 16:23, Tanu Kaskinen wrote: > On Tue, 2017-07-18 at 20:16 +0200, Georg Chini wrote: >> On 17.07.2017 19:44, Tanu Kaskinen wrote: >>> On Sun, 2017-07-16 at 14:35 +0200, Georg Chini wrote: >>>> On 04.07.2017 15:38, Tanu Kaskinen wrote: >>>>> It looks racy indeed, so some check should be added as you say. When >>>>> shutting down or changing the profile, stop_thread() is always called. >>>>> stop_thread() calls pa_thread_mq_done(), and this is where queued >>>>> messages are processed. The transport hasn't been released yet at this >>>>> point, but I think the transport release can be moved to happen before >>>>> pa_thread_mq_done(), then you can check if u->transport is NULL. >>>>> >>>> Mh, after looking at the code, I don't see the race condition. Let's >>>> assume we went through transport_acquire() and sent a >>>> BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING message. This >>>> means we successfully acquired the transport. Now something >>>> happens that leads to a thread shutdown before the message is >>>> processed. This can either be a profile change or the remote end >>>> unexpectedly closing the connection. In all cases stop_thread() >>>> will be called. In stop_thread() the outstanding message will be >>>> processed and the transport set to playing in pa_thread_mq_done(). >>>> This does not really matter, because the transport is released >>>> immediately after this through transport_release(). It just reflects - >>>> with a little delay - what happened in reality. >>>> >>>> In my opinion we would only have a race condition if it was possible >>>> that the transport was released before the message was processed >>>> but I do not see how this could happen. >>>> >>>> But maybe I just did not see the point (again), so please correct me >>>> if I'm wrong. >>> You have a point - I can't point to any specific thing that will >>> definitely break. However, the IO thread has already been torn down >>> when the SET_TRANSPORT_PLAYING message is processed, and I'm not sure >>> if setting the transport state to PLAYING is safe in that situation. >>> pa_bluetooth_transport_set_state() will fire some hooks, and I didn't >>> trace down what happens in those hooks. It seems safer to tear down the >>> transport while the IO thread is still running. >>> >> After another look I understand even less ... >> Doesn't pa_thread_mq_done() process the final messages for the I/O >> thread and not for the main thread? The messages we are talking >> about are messages from the I/O thread to the main thread and are >> therefore processed in the main thread. This can't happen while the >> main thread is executing stop_thread(), so from that perspective >> it should not matter where in stop_thread() the transport is set to >> NULL (unless my assumption concerning pa_thread_mq_done() is >> wrong). >> On the other hand I think it should only be set to NULL after all I/O >> thread messages have been processed, which is after >> pa_thread_mq_done(), so I still believe releasing the transport after >> the call is correct. >> >> Now however I wonder if there is the possibility that there are still >> pending BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING >> messages after stop_thread() has been called. This is should be no >> problem if pa_bluetooth_transport_set_state() just returns if the >> transport is NULL (currently it asserts). >> >> Do you still think I should move the transport release before >> pa_thread_mq_done()? > You're right, pa_thread_mq_done() doesn't process the messages that are > sent to the main thread, contrary to what I thought. Moving the > transport release isn't necessary. > > Your suggestion of changing pa_bluetooth_transport_set_state() doesn't > seem safe: when changing profiles, u->transport will not be NULL when > pa_bluetooth_transport_set_state() is called, but the > SET_TRANSPORT_PLAYING message was meant for the old transport, not the > new one. I think the message needs to identify the transport that > should be set to PLAYING. When the message is processed, the transport > state should be set only if the current transport matches the transport > identified by the message. > Because stop_thread() calls transport_release(), transport_acquired will be set to false. On the other hand, transport_acquire() sets it to true before sending the message. So would it be enough to just check if transport_acquired is still true?
