RESOLVED FIXED 221894
RemoteAudioSourceProviderManager is accessed in non-thread-safe manner
https://bugs.webkit.org/show_bug.cgi?id=221894
Summary RemoteAudioSourceProviderManager is accessed in non-thread-safe manner
Kimmo Kinnunen
Reported 2021-02-15 02:43:49 PST
RemoteAudioSourceProviderManager is accessed in non-thread-safe manner The class calls Connection::addWorkQueueMessageReceiver() during its construction, passing 'this' as the receiver. The virtual function pointer is not fully constructed during the call time. Connection may start dispatching calls to the work queue. The work queue may dispatch the task function through that pointer immediately after the call. If a message comes during the constructor call, it may be dispatched during the constructor still running, before the virtual function pointer being correctly set up. Accessing virtual function pointer is not thread safe until it is fully constructed. The class calls Connection::removeWorkQueueMessageReceiver() during its destruction. Connection may still dispatch calls to the work queue and the work queue through the passed in virtual function pointer. The virtual pointer is not as expected anymore at that point, is not thread-safe to read. Upon dispatch, the connection will ref the WorkQueueMessageReceiver. Since the instance might already be in destructor, this is a ref count increase on a destroyed object.
Attachments
Patch (8.48 KB, patch)
2021-02-15 02:56 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Patch (8.53 KB, patch)
2021-02-15 04:40 PST, Kimmo Kinnunen
no flags
update changelog (8.04 KB, patch)
2021-02-15 12:17 PST, Kimmo Kinnunen
no flags
update name (8.03 KB, patch)
2021-02-16 00:33 PST, Kimmo Kinnunen
no flags
Kimmo Kinnunen
Comment 1 2021-02-15 02:56:00 PST
Kimmo Kinnunen
Comment 2 2021-02-15 04:40:01 PST
Eric Carlson
Comment 3 2021-02-15 08:37:02 PST
Comment on attachment 420306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review > Source/WebKit/ChangeLog:8 > + Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor. I don't see this change. Did you forgot to include it in the patch?
Chris Dumez
Comment 4 2021-02-15 10:51:02 PST
Comment on attachment 420306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review >> Source/WebKit/ChangeLog:8 >> + Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor. > > I don't see this change. Did you forgot to include it in the patch? I think this was done in bug 221891. This changelog probably needs updating.
Chris Dumez
Comment 5 2021-02-15 10:56:38 PST
Comment on attachment 420306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review > Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88 > + m_audioSourceProviderManager->disconnectGPUProcess(); This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code?
Kimmo Kinnunen
Comment 6 2021-02-15 12:17:02 PST
Comment on attachment 420306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review Thanks for reviewing! >>> Source/WebKit/ChangeLog:8 >>> + Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor. >> >> I don't see this change. Did you forgot to include it in the patch? > > I think this was done in bug 221891. This changelog probably needs updating. Ah, thanks! I've made this change in so many ways now to pass the review that I got the patches and changes done mixed up. Removed the comment, the change changes only the destructor. >> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88 >> + m_audioSourceProviderManager->disconnectGPUProcess(); > > This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code? You mean the RemoteAudioSourceProviderManager::disconnectGPUProcess() added in the next hunk below? Or GPUProcessConnection::~GPUProcessConnection()?
Kimmo Kinnunen
Comment 7 2021-02-15 12:17:17 PST
Created attachment 420346 [details] update changelog
Chris Dumez
Comment 8 2021-02-15 12:19:37 PST
(In reply to Kimmo Kinnunen from comment #6) > Comment on attachment 420306 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420306&action=review > > Thanks for reviewing! > > >>> Source/WebKit/ChangeLog:8 > >>> + Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor. > >> > >> I don't see this change. Did you forgot to include it in the patch? > > > > I think this was done in bug 221891. This changelog probably needs updating. > > Ah, thanks! I've made this change in so many ways now to pass the review > that I got the patches and changes done mixed up. > Removed the comment, the change changes only the destructor. > > >> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88 > >> + m_audioSourceProviderManager->disconnectGPUProcess(); > > > > This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code? > > You mean the RemoteAudioSourceProviderManager::disconnectGPUProcess() added > in the next hunk below? > Or GPUProcessConnection::~GPUProcessConnection()? Yes, I had missed it.
Kimmo Kinnunen
Comment 9 2021-02-16 00:33:34 PST
Created attachment 420432 [details] update name
EWS
Comment 10 2021-02-16 10:08:14 PST
Committed r272915: <https://commits.webkit.org/r272915> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420432 [details].
Radar WebKit Bug Importer
Comment 11 2021-02-16 10:09:13 PST
Note You need to log in before you can comment on or make changes to this bug.