[fuchsia] Reject duplicate sysmem buffer collection ids [chromium/src : main]
David Song (Gerrit)
David Song added 1 comment![]( "Open in Gerrit")
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Wouldn't this introduce a TOCTOU issue?
Related details
- Bailey Myers-Morgan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan added 1 comment![]( "Open in Gerrit")
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Wouldn't this introduce a TOCTOU issue?
Yes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
Related details
- David Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Song (Gerrit)
David Song added 1 comment![]( "Open in Gerrit")
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Bailey Myers-MorganWouldn't this introduce a TOCTOU issue?
Yes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
- Bailey Myers-Morgan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan added 1 comment![]( "Open in Gerrit")
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Bailey Myers-MorganWouldn't this introduce a TOCTOU issue?
David SongYes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
We can remove this block then, right?
PTAL -- now using `pending_registrations_` to track koids immediately. I believe that should prevent double-registration of collections?
w/r/t returnning `nullptr` rather than `void`, Jetski says:
The public interface `ImportSysmemBufferCollection` is defined to return `scoped_refptr<FlatlandSysmemBufferCollection>`. Because of this signature, we return `nullptr` on failure paths so that calling code (which expects a ref-pointer) knows the import failed and doesn't try to use an invalid reference.
Related details
- David Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Song (Gerrit)
David Song added 2 comments![]( "Open in Gerrit")
std::unordered_set<zx_koid_t> pending_registrations_
GUARDED_BY(collections_lock_);I don't think we need to introduce another unordered_set here. We can store a `nullptr` in `collections_`, which holds the key until it's initialized.
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Bailey Myers-MorganWouldn't this introduce a TOCTOU issue?
David SongYes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
Bailey Myers-MorganWe can remove this block then, right?
PTAL -- now using `pending_registrations_` to track koids immediately. I believe that should prevent double-registration of collections?
w/r/t returnning `nullptr` rather than `void`, Jetski says:
The public interface `ImportSysmemBufferCollection` is defined to return `scoped_refptr<FlatlandSysmemBufferCollection>`. Because of this signature, we return `nullptr` on failure paths so that calling code (which expects a ref-pointer) knows the import failed and doesn't try to use an invalid reference.
Flatland is a Fuchsia concept, and we can change its public interface. There's only 1 caller of `ImportSysmemBufferCollection`, and it immediately drops the return value, so it's already not doing what Jetski is saying.
Related details
- Bailey Myers-Morgan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan added 3 comments![]( "Open in Gerrit")
std::unordered_set<zx_koid_t> pending_registrations_
GUARDED_BY(collections_lock_);I don't think we need to introduce another unordered_set here. We can store a `nullptr` in `collections_`, which holds the key until it's initialized.
Oh, that's clever! I didn't think of that.
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Bailey Myers-MorganWouldn't this introduce a TOCTOU issue?
David SongYes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
Bailey Myers-MorganWe can remove this block then, right?
David SongPTAL -- now using `pending_registrations_` to track koids immediately. I believe that should prevent double-registration of collections?
w/r/t returnning `nullptr` rather than `void`, Jetski says:
The public interface `ImportSysmemBufferCollection` is defined to return `scoped_refptr<FlatlandSysmemBufferCollection>`. Because of this signature, we return `nullptr` on failure paths so that calling code (which expects a ref-pointer) knows the import failed and doesn't try to use an invalid reference.
Flatland is a Fuchsia concept, and we can change its public interface. There's only 1 caller of `ImportSysmemBufferCollection`, and it immediately drops the return value, so it's already not doing what Jetski is saying.
Done. I was hesitant to change the interface itself; I suppose the only concern would be someone trying to use the refptr in the future, and that seems pretty unlikely. And they could also just as easily change it back.
void FlatlandSysmemBufferManager::RegisterCollection(PTAL here--I switched this back to `void` since the `bool` return didn't actually address the TOCTOU issue. Let me know if I'm missing something, but I think the early return should work the same as `return False`.
Related details
- David Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Song (Gerrit)
David Song voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Song
| Code-Review | +1 |
2 comments
{
base::AutoLock auto_lock(collections_lock_);
if (collections_.contains(koid.value())) {
return nullptr;
}
}Bailey Myers-MorganWouldn't this introduce a TOCTOU issue?
David SongYes, I think you're right. I've refactored to return a bool rather than void so the code should know better when to properly destroy the collection. Does this look like it'll resolve your concern?
Bailey Myers-MorganWe can remove this block then, right?
David SongPTAL -- now using `pending_registrations_` to track koids immediately. I believe that should prevent double-registration of collections?
w/r/t returnning `nullptr` rather than `void`, Jetski says:
The public interface `ImportSysmemBufferCollection` is defined to return `scoped_refptr<FlatlandSysmemBufferCollection>`. Because of this signature, we return `nullptr` on failure paths so that calling code (which expects a ref-pointer) knows the import failed and doesn't try to use an invalid reference.
Bailey Myers-MorganFlatland is a Fuchsia concept, and we can change its public interface. There's only 1 caller of `ImportSysmemBufferCollection`, and it immediately drops the return value, so it's already not doing what Jetski is saying.
Done. I was hesitant to change the interface itself; I suppose the only concern would be someone trying to use the refptr in the future, and that seems pretty unlikely. And they could also just as easily change it back.
LGTM.
void FlatlandSysmemBufferManager::RegisterCollection(PTAL here--I switched this back to `void` since the `bool` return didn't actually address the TOCTOU issue. Let me know if I'm missing something, but I think the early return should work the same as `return False`.
Agreed on switching back to `void` here. We can go further and `CHECK` our invariant that the `koid` is present in the `collections_` map as `nullptr`.
Related details
- Bailey Myers-Morgan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan added 1 comment![]( "Open in Gerrit")
void FlatlandSysmemBufferManager::RegisterCollection(David SongPTAL here--I switched this back to `void` since the `bool` return didn't actually address the TOCTOU issue. Let me know if I'm missing something, but I think the early return should work the same as `return False`.
Agreed on switching back to `void` here. We can go further and `CHECK` our invariant that the `koid` is present in the `collections_` map as `nullptr`.
That's a clever idea. Could you quickly PTAL at this change (patchset 6 to 7)? I'm pretty unfamiliar with CHECKs so I want to make sure I replaced the right thing. 😊
Related details
- David Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Song (Gerrit)
David Song voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Song
| Code-Review | +1 |
1 comment
void FlatlandSysmemBufferManager::RegisterCollection(David SongPTAL here--I switched this back to `void` since the `bool` return didn't actually address the TOCTOU issue. Let me know if I'm missing something, but I think the early return should work the same as `return False`.
Bailey Myers-MorganAgreed on switching back to `void` here. We can go further and `CHECK` our invariant that the `koid` is present in the `collections_` map as `nullptr`.
That's a clever idea. Could you quickly PTAL at this change (patchset 6 to 7)? I'm pretty unfamiliar with CHECKs so I want to make sure I replaced the right thing. 😊
- Bailey Myers-Morgan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bailey Myers-Morgan (Gerrit)
Bailey Myers-Morgan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/platform/flatland/flatland_sysmem_buffer_manager_unittest.cc
Insertions: 6, Deletions: 1.
@@ -31,6 +31,11 @@
it->second.get() == collection_ptr;
}
+ void RegisterCollectionDirect(
+ scoped_refptr<FlatlandSysmemBufferCollection> collection) {
+ manager_.RegisterCollection(std::move(collection));
+ }
+
void ClearCollections() {
base::AutoLock auto_lock(manager_.collections_lock_);
manager_.collections_.clear();
@@ -60,7 +65,7 @@
ASSERT_EQ(collection_a->id(), collection_b->id());
EXPECT_TRUE(RegisterCollection(collection_a));
- EXPECT_DEATH(manager_.RegisterCollection(collection_b), "");
+ EXPECT_DEATH(RegisterCollectionDirect(collection_b), "");
// The first registered collection must remain in the registry; a second
// collection with the same id must not displace it.
```
Change information
[fuchsia] Reject duplicate sysmem buffer collection ids
FlatlandSysmemBufferManager keys its collection registry by the koid of
the caller-supplied eventpair. Registering a second collection that uses
a duplicate of an already-registered handle would overwrite the existing
map entry, dropping the manager's reference to the original collection
while its handle watch remained armed.
ImportSysmemBufferCollection() now rejects handles whose koid is already
registered, and RegisterCollection() uses insert() semantics so an
existing entry is never displaced.
FlatlandSysmemBufferCollection::OnZxHandleSignalled() now moves its
release callbacks to the stack before running them instead of taking a
self reference, so it never touches the refcount of an object whose last
reference may already have been released.
- M ui/ozone/platform/flatland/BUILD.gn
- M ui/ozone/platform/flatland/flatland_sysmem_buffer_collection.cc
- M ui/ozone/platform/flatland/flatland_sysmem_buffer_manager.cc
- M ui/ozone/platform/flatland/flatland_sysmem_buffer_manager.h
- A ui/ozone/platform/flatland/flatland_sysmem_buffer_manager_unittest.cc
Code-Review: +1 by David Song
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |