Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006883002:
Gracefully handle dirtying of audio nodes while processing current set. (Closed)
DescriptionGracefully handle dirtying of audio nodes while processing current set.
When processing the set of dirty output nodes, nodes further down the
chain may be marked as dirty as a result. Take that into account
when iterating over the current set.
R=hoch
BUG=610643, 613902
Committed: https://crrev.com/7b953ca0869bb4f2910b0f834b0ce7ad89166445
Cr-Commit-Position: refs/heads/master@{#395643}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 19 (5 generated)
sigbjornf@opera.com changed reviewers: + hongchan@chromium.org, rtoy@chromium.org
please take a look. I'm not sure what's preferable here: have the loop be shallow and only process the current dirty set, or process until the set has been completely drained. (The initial/previous analysis of what caused 610643 was completely off.. but its defensive fix is independently worthwhile, i think.)
Quite clever! If this pattern is robust/optimum for this type of operation, we might want to do this at other places. But I have one question about swapping HashSet: https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: m_dirtyAudioNodeOutputs.swap(dirtyOutputs); What if this swapping happens when the audio thread is accessing m_dirtyAudionodeOutputs? Is this guaranteed to be atomic?
https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: m_dirtyAudioNodeOutputs.swap(dirtyOutputs); On 2016/05/23 20:43:34, hoch wrote: > What if this swapping happens when the audio thread is accessing > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > No, the calling context will have to guarantee exclusive access. I thought handleDeferredTasks() was only performed on the audio thread?
On 2016/05/23 21:06:30, sof wrote: > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: > m_dirtyAudioNodeOutputs.swap(dirtyOutputs); > On 2016/05/23 20:43:34, hoch wrote: > > What if this swapping happens when the audio thread is accessing > > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > > > > No, the calling context will have to guarantee exclusive access. I thought > handleDeferredTasks() was only performed on the audio thread? I think we have to look for both sides: DeferredTaskHandler::markAudioNodeOutputDirty (accessing m_dirtyAudioNodeOutputs from the main thread) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... DeferredTaskHandler::handleDirtyAudioNodeOutputs (accessing m_dirtyAudioNodeOutputs from the audio thread) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Two observations: 1) Weirdly enough, the crash report says this is UAF issue at handleDirtyAudioNodeOutputs(), not the data race. 2) The test case creates 2 AudioContexts in parallel and fuzzes them with many different nodes. But the creation happens at the beginning, just once. So the test is not creating/destroying tons of audio contexts. I believe this issue still needs a bit more of investigation.
On 2016/05/23 21:23:08, hoch wrote: > On 2016/05/23 21:06:30, sof wrote: > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp > (right): > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: > > m_dirtyAudioNodeOutputs.swap(dirtyOutputs); > > On 2016/05/23 20:43:34, hoch wrote: > > > What if this swapping happens when the audio thread is accessing > > > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > > > > > > > No, the calling context will have to guarantee exclusive access. I thought > > handleDeferredTasks() was only performed on the audio thread? > > I think we have to look for both sides: > > DeferredTaskHandler::markAudioNodeOutputDirty (accessing m_dirtyAudioNodeOutputs > from the main thread) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > DeferredTaskHandler::handleDirtyAudioNodeOutputs (accessing > m_dirtyAudioNodeOutputs from the audio thread) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Two observations: > > 1) Weirdly enough, the crash report says this is UAF issue at > handleDirtyAudioNodeOutputs(), not the data race. > 2) The test case creates 2 AudioContexts in parallel and fuzzes them with many > different nodes. But the creation happens at the beginning, just once. So the > test is not creating/destroying tons of audio contexts. > > I believe this issue still needs a bit more of investigation. The hash table is expanded and rehashed, freeing the previous backing store. The iterator holds a reference to the backing store, hence UAF.
On 2016/05/23 21:29:27, sof wrote: > On 2016/05/23 21:23:08, hoch wrote: > > On 2016/05/23 21:06:30, sof wrote: > > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp > > (right): > > > > > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: > > > m_dirtyAudioNodeOutputs.swap(dirtyOutputs); > > > On 2016/05/23 20:43:34, hoch wrote: > > > > What if this swapping happens when the audio thread is accessing > > > > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > > > > > > > > > > No, the calling context will have to guarantee exclusive access. I thought > > > handleDeferredTasks() was only performed on the audio thread? > > > > I think we have to look for both sides: > > > > DeferredTaskHandler::markAudioNodeOutputDirty (accessing > m_dirtyAudioNodeOutputs > > from the main thread) > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > DeferredTaskHandler::handleDirtyAudioNodeOutputs (accessing > > m_dirtyAudioNodeOutputs from the audio thread) > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Two observations: > > > > 1) Weirdly enough, the crash report says this is UAF issue at > > handleDirtyAudioNodeOutputs(), not the data race. > > 2) The test case creates 2 AudioContexts in parallel and fuzzes them with many > > different nodes. But the creation happens at the beginning, just once. So the > > test is not creating/destroying tons of audio contexts. > > > > I believe this issue still needs a bit more of investigation. > > The hash table is expanded and rehashed, freeing the previous backing store. The > iterator holds a reference to the backing store, hence UAF. Or can we use copyToVector? We normally use copyToVector when we want to iterate things without worrying about any modification to the original hash set.
On 2016/05/23 21:59:37, haraken wrote: > On 2016/05/23 21:29:27, sof wrote: > > On 2016/05/23 21:23:08, hoch wrote: > > > On 2016/05/23 21:06:30, sof wrote: > > > > > > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > > > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > > > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: > > > > m_dirtyAudioNodeOutputs.swap(dirtyOutputs); > > > > On 2016/05/23 20:43:34, hoch wrote: > > > > > What if this swapping happens when the audio thread is accessing > > > > > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > > > > > > > > > > > > > No, the calling context will have to guarantee exclusive access. I thought > > > > handleDeferredTasks() was only performed on the audio thread? > > > > > > I think we have to look for both sides: > > > > > > DeferredTaskHandler::markAudioNodeOutputDirty (accessing > > m_dirtyAudioNodeOutputs > > > from the main thread) > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > DeferredTaskHandler::handleDirtyAudioNodeOutputs (accessing > > > m_dirtyAudioNodeOutputs from the audio thread) > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > Two observations: > > > > > > 1) Weirdly enough, the crash report says this is UAF issue at > > > handleDirtyAudioNodeOutputs(), not the data race. > > > 2) The test case creates 2 AudioContexts in parallel and fuzzes them with > many > > > different nodes. But the creation happens at the beginning, just once. So > the > > > test is not creating/destroying tons of audio contexts. > > > > > > I believe this issue still needs a bit more of investigation. > > > > The hash table is expanded and rehashed, freeing the previous backing store. > The > > iterator holds a reference to the backing store, hence UAF. > > Or can we use copyToVector? We normally use copyToVector when we want to iterate > things without worrying about any modification to the original hash set. I don't think that's a good idea for process-and-clear patterns. The normal case here is that you won't be adding new nodes (otherwise this bug would have shown itself much earlier), so creating a new vector allocation to hold the any-order-goes set's backing store seems like a waste.
+cc: haraken
On 2016/05/23 21:23:08, hoch wrote: > On 2016/05/23 21:06:30, sof wrote: > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp > (right): > > > > > https://codereview.chromium.org/2006883002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:135: > > m_dirtyAudioNodeOutputs.swap(dirtyOutputs); > > On 2016/05/23 20:43:34, hoch wrote: > > > What if this swapping happens when the audio thread is accessing > > > m_dirtyAudionodeOutputs? Is this guaranteed to be atomic? > > > > > > > No, the calling context will have to guarantee exclusive access. I thought > > handleDeferredTasks() was only performed on the audio thread? > > I think we have to look for both sides: > > DeferredTaskHandler::markAudioNodeOutputDirty (accessing m_dirtyAudioNodeOutputs > from the main thread) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > DeferredTaskHandler::handleDirtyAudioNodeOutputs (accessing > m_dirtyAudioNodeOutputs from the audio thread) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > The main thread uses of markAudioNodeOutputDirty() (by way setNumberOfChannels()) are done while holding the context lock, so exclusion on m_dirtyAudioNodeOutputs is provided that way. We shouldn't add additional locking.
lgtm. Do you think this pattern is safe for other storage members in DeferredTaskHandler?
On 2016/05/24 17:08:58, hoch wrote: > lgtm. > > Do you think this pattern is safe for other storage members in > DeferredTaskHandler? It appears so, perhaps we should encourage this pattern more in general? Unexpected mutation of hashed collections while iterating is a fairly unpleasant bug, and there's been a number of such bugs in our codebase.
Description was changed from ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R= BUG=610643, 613902 ========== to ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R=hoch BUG=610643, 613902 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006883002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006883002/1
Message was sent while issue was closed.
Description was changed from ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R=hoch BUG=610643, 613902 ========== to ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R=hoch BUG=610643, 613902 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R=hoch BUG=610643, 613902 ========== to ========== Gracefully handle dirtying of audio nodes while processing current set. When processing the set of dirty output nodes, nodes further down the chain may be marked as dirty as a result. Take that into account when iterating over the current set. R=hoch BUG=610643, 613902 Committed: https://crrev.com/7b953ca0869bb4f2910b0f834b0ce7ad89166445 Cr-Commit-Position: refs/heads/master@{#395643} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/7b953ca0869bb4f2910b0f834b0ce7ad89166445 Cr-Commit-Position: refs/heads/master@{#395643} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
