Implement MediaConnectionManager for media connection limits. [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
Still waiting on CQ results, but this is good enough to start looking at.
Related details
- Ted (Chromium) Meyer
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 4 comments![]( "Open in Gerrit")
// May have been removed by MaybeReleaseDeferredConnections().Shouldn't this be NOTREACHED? Where is `loader` owned if not in one of these maps?
auto& pending_deque = pending_it->value;I'm not sure what the purpose of this is? removing it would also fix the UAF, as `deferred_loaders_per_origin_.find(origin)` can become part of the while loop.
while (!deferred_deque.empty() && !pending_deque.empty()) {There's a UAF in this call stack:
```
MediaConnectionManager::MaybeReleaseDeferredConnections
ManagedWebAssociatedURLLoader::Abort
WebAssociatedURLLoaderClient::DidFail
ManagedWebAssociatedURLLoader::~ManagedWebAssociatedURLLoader
MediaConnectionManager::ReleaseLoader
MediaConnectionManager::ClearOriginIfEmpty
```
which deletes the storage for `deferred_queue` if it's got size==1
origin_it->value.erase(loader_it);I think it makes sense to add `if (origin_it.empty()) { map.erase(origin); }` here, rather than having the `ClearOriginIfEmpty` function. I'm _pretty_ sure they're being correctly erased now, though in the future changes might make that more uncertain.
Related details
- Dale Curtis
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
// May have been removed by MaybeReleaseDeferredConnections().Shouldn't this be NOTREACHED? Where is `loader` owned if not in one of these maps?
When `MaybeReleaseDeferredConnections()` is called and removes the loader from `deferred_loaders_per_origin_` this function is still called, so it may have already been removed. That's what the comment is supposed to mean, happy to make it clearer if you have suggestions.
I'm not sure what the purpose of this is? removing it would also fix the UAF, as `deferred_loaders_per_origin_.find(origin)` can become part of the while loop.
We don't want to always release all deferred connections, we only want to release enough of them to start any pending loaders. But yeah, it should be part of the while loop.
while (!deferred_deque.empty() && !pending_deque.empty()) {There's a UAF in this call stack:
```
MediaConnectionManager::MaybeReleaseDeferredConnections
ManagedWebAssociatedURLLoader::Abort
WebAssociatedURLLoaderClient::DidFail
ManagedWebAssociatedURLLoader::~ManagedWebAssociatedURLLoader
MediaConnectionManager::ReleaseLoader
MediaConnectionManager::ClearOriginIfEmpty
```which deletes the storage for `deferred_queue` if it's got size==1
Done
I think it makes sense to add `if (origin_it.empty()) { map.erase(origin); }` here, rather than having the `ClearOriginIfEmpty` function. I'm _pretty_ sure they're being correctly erased now, though in the future changes might make that more uncertain.
This will result in churning between empty and non-empty deques as loaders move in/out of deferred states. Synthetic testing shows about a 25% overhead.
ReleaseLoader() must always be called, so this should be pretty bulletproof.
Related details
- Ted (Chromium) Meyer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |