[fuchsia] Make ClientNativePixmapFuchsia thread-safe [chromium/src : main]
Wez (Gerrit)
Wez added 7 comments![]( "Open in Gerrit")
This seems fine, but the only caller of `ClientNativePixmap::Map()` seems to be a `LegacyCpuMemoryBuffer` code path (see https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/common/legacy_gpu_memory_buffer_for_video.cc;l=69) which applies locking around the operations.
So it seems to be the case that (1) `ClientNativePixmap` is deprecated and (2) the interface's implementations need _not_ be _thread-safe_ (i.e. internally synchrony ized), but must be _thread-compatible_ or _thread-unsafe_ (i.e. OK to use with appropriate caller synchronization). Our current `ClientNativePixmapFuchsia` has a _thread-compatible_ implementation but imposes _thread-hostile_ sequence checks.
# DO NOT SUBMIT
dcheck_always_on = trueFYI, you can just hack the default for the arg in-place, leaving the `declare_args()` otherwise intact, for tests like this, in future 😊
// Functions in this class need to be thread-safe, they can be called from
// different threads.Is this true, or is it only the case that the `Map()` and `Unmap()` (and `GetNumberOfPlanes()` etc?) operations may be called on a different sequence from the setup/teardown sequence?
base::AutoLock lock(lock_);This isn't incorrect per-se, but it should not be necessary, since by definition if we have accesses to `this` racing with destruction then we have bigger problems.
If possible I think it would be clearer to have two `SEQUENCE_CHECKER()`s in this object, one checked on construction & destruction, and the other initial un-bound, and checked in `Map()`, `Unmap()`, etc - that would express the constraint that the object _can_ be created & torn-down on a different sequence to the one on which it is used, but that it must be used from a consistent sequence, which I think is the actual property required here?
base::AutoLock lock(lock_);
return handle_.planes.size();nit: This is only working via the `handle_`, which is `const`, so presumably no need for locking here?
base::AutoLock lock(lock_);`handle_` is `const`, so no need to lock here?
base::AutoLock lock(lock_);No need for lock because `handle_` is `const`.
Related details
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 2 comments![]( "Open in Gerrit")
This seems fine, but the only caller of `ClientNativePixmap::Map()` seems to be a `LegacyCpuMemoryBuffer` code path (see https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/common/legacy_gpu_memory_buffer_for_video.cc;l=69) which applies locking around the operations.
So it seems to be the case that (1) `ClientNativePixmap` is deprecated and (2) the interface's implementations need _not_ be _thread-safe_ (i.e. internally synchrony ized), but must be _thread-compatible_ or _thread-unsafe_ (i.e. OK to use with appropriate caller synchronization). Our current `ClientNativePixmapFuchsia` has a _thread-compatible_ implementation but imposes _thread-hostile_ sequence checks.
Emmm, interesting, I never saw this class in the stack trace before. But considering you fixed the destruction sequence, I am rerunning https://crrev.com/i/8394026 and will get the latest failures if any.
base::AutoLock lock(lock_);`handle_` is `const`, so no need to lock here?
Ah, yes, I think I messed two different intentions altogether; the const was also added in this CL. So yes, locking in this function is not necessary.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
base::AutoLock lock(lock_);This isn't incorrect per-se, but it should not be necessary, since by definition if we have accesses to `this` racing with destruction then we have bigger problems.
If possible I think it would be clearer to have two `SEQUENCE_CHECKER()`s in this object, one checked on construction & destruction, and the other initial un-bound, and checked in `Map()`, `Unmap()`, etc - that would express the constraint that the object _can_ be created & torn-down on a different sequence to the one on which it is used, but that it must be used from a consistent sequence, which I think is the actual property required here?
It's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below. Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
Related details
- Wez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wez (Gerrit)
Wez added 1 comment![]( "Open in Gerrit")
base::AutoLock lock(lock_);Zijie HeThis isn't incorrect per-se, but it should not be necessary, since by definition if we have accesses to `this` racing with destruction then we have bigger problems.
If possible I think it would be clearer to have two `SEQUENCE_CHECKER()`s in this object, one checked on construction & destruction, and the other initial un-bound, and checked in `Map()`, `Unmap()`, etc - that would express the constraint that the object _can_ be created & torn-down on a different sequence to the one on which it is used, but that it must be used from a consistent sequence, which I think is the actual property required here?
It's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below. Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
It's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below.
If there is a `Map()` or `Unmap()` in-flight while the destructor is running then all bets are already off - adding locking just means that the in-flight call could run _after_ the destructor completes and releases the lock, at which point the object is gone and you have UaF.
As-is the logic will actually deadlock, unless the `Lock` implementation is re-entrant, since both the destructor and `Unmap()` are locking the object.
Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
No other operations can be running on other threads at the point when the destructor executes, or all bets are off. There are special-cases where that could work, e.g. the object itself has an internal thread and blocks in the destructor until it exits, but that's not the situation here.
Even if we did need to `AutoLock` to access `mapping_`, to appease the threading annotations checker, `mapping_` has an invariant (in the current implementation) that it can only change from not-set to set, since `Unmap()` flushes the cache without actually un-mapping the memory. So we can lock, check the value, and unlock and (because we know it would be intrinsically unsafe for `Map()` to run concurrently with destruction) we can then unlock before doing the conditional work.
However, I suspect that the caller just creates/destroys on one sequence and map/unmaps on a different one, and takes care of ensuring that is safe, so that we just need to relax the sequence-checker constraint that currently regards this class as thread-hostile.
Related details
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
base::AutoLock lock(lock_);Zijie HeThis isn't incorrect per-se, but it should not be necessary, since by definition if we have accesses to `this` racing with destruction then we have bigger problems.
If possible I think it would be clearer to have two `SEQUENCE_CHECKER()`s in this object, one checked on construction & destruction, and the other initial un-bound, and checked in `Map()`, `Unmap()`, etc - that would express the constraint that the object _can_ be created & torn-down on a different sequence to the one on which it is used, but that it must be used from a consistent sequence, which I think is the actual property required here?
WezIt's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below. Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
It's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below.
If there is a `Map()` or `Unmap()` in-flight while the destructor is running then all bets are already off - adding locking just means that the in-flight call could run _after_ the destructor completes and releases the lock, at which point the object is gone and you have UaF.
As-is the logic will actually deadlock, unless the `Lock` implementation is re-entrant, since both the destructor and `Unmap()` are locking the object.
Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
No other operations can be running on other threads at the point when the destructor executes, or all bets are off. There are special-cases where that could work, e.g. the object itself has an internal thread and blocks in the destructor until it exits, but that's not the situation here.
Even if we did need to `AutoLock` to access `mapping_`, to appease the threading annotations checker, `mapping_` has an invariant (in the current implementation) that it can only change from not-set to set, since `Unmap()` flushes the cache without actually un-mapping the memory. So we can lock, check the value, and unlock and (because we know it would be intrinsically unsafe for `Map()` to run concurrently with destruction) we can then unlock before doing the conditional work.
However, I suspect that the caller just creates/destroys on one sequence and map/unmaps on a different one, and takes care of ensuring that is safe, so that we just need to relax the sequence-checker constraint that currently regards this class as thread-hostile.
I never thought a project as large as chromium would prefer non-nestable locks, it's way too easy to be abused, comparing to the performance benefit we achieve. But I do take a look at the code and notice that it is not guaranteed nestable. Anyway 😂.
I agree the lock in the destructor is overreacting, I assume `zx::vmar::root_self()->unmap` needs no thread-protection.
But using two sequence-checkers does not work, we do know that the destructor and Map() are running on different threads.
On the other hand, it's a great idea to relax the sequence-checker constraint. It fails and serves no purpose. I can quickly remove the sequence-checker, enable the DCHECK in our builders, and come back for a proper fix.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wez (Gerrit)
Wez added 1 comment![]( "Open in Gerrit")
base::AutoLock lock(lock_);Zijie HeThis isn't incorrect per-se, but it should not be necessary, since by definition if we have accesses to `this` racing with destruction then we have bigger problems.
If possible I think it would be clearer to have two `SEQUENCE_CHECKER()`s in this object, one checked on construction & destruction, and the other initial un-bound, and checked in `Map()`, `Unmap()`, etc - that would express the constraint that the object _can_ be created & torn-down on a different sequence to the one on which it is used, but that it must be used from a consistent sequence, which I think is the actual property required here?
WezIt's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below. Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
Zijie HeIt's possible that constructor and destructor is using a different sequence than Map() & Unmap(), but the problem is that the destructor calls Unmap() conditionally below.
If there is a `Map()` or `Unmap()` in-flight while the destructor is running then all bets are already off - adding locking just means that the in-flight call could run _after_ the destructor completes and releases the lock, at which point the object is gone and you have UaF.
As-is the logic will actually deadlock, unless the `Lock` implementation is re-entrant, since both the destructor and `Unmap()` are locking the object.
Also technically speaking, accessing mapping_ needs to be thread protected if Map() can be executed in a different thread.
No other operations can be running on other threads at the point when the destructor executes, or all bets are off. There are special-cases where that could work, e.g. the object itself has an internal thread and blocks in the destructor until it exits, but that's not the situation here.
Even if we did need to `AutoLock` to access `mapping_`, to appease the threading annotations checker, `mapping_` has an invariant (in the current implementation) that it can only change from not-set to set, since `Unmap()` flushes the cache without actually un-mapping the memory. So we can lock, check the value, and unlock and (because we know it would be intrinsically unsafe for `Map()` to run concurrently with destruction) we can then unlock before doing the conditional work.
However, I suspect that the caller just creates/destroys on one sequence and map/unmaps on a different one, and takes care of ensuring that is safe, so that we just need to relax the sequence-checker constraint that currently regards this class as thread-hostile.
I never thought a project as large as chromium would prefer non-nestable locks, it's way too easy to be abused, comparing to the performance benefit we achieve. But I do take a look at the code and notice that it is not guaranteed nestable. Anyway 😂.
I agree the lock in the destructor is overreacting, I assume `zx::vmar::root_self()->unmap` needs no thread-protection.But using two sequence-checkers does not work, we do know that the destructor and Map() are running on different threads.
On the other hand, it's a great idea to relax the sequence-checker constraint. It fails and serves no purpose. I can quickly remove the sequence-checker, enable the DCHECK in our builders, and come back for a proper fix.
[snip]
> But using two sequence-checkers does not work, we do know that the destructor and Map() are running on different threads.
That's why I'm suggesting using two distinct sequence-checkers - one to ensure same-sequence use of map/unmap and a separate one for create/destroy. The map/unmap one would likely need to be "detached" on destruction, to prevent it from `DCHECK`ing, though.
On the other hand, it's a great idea to relax the sequence-checker constraint. It fails and serves no purpose. I can quickly remove the sequence-checker, enable the DCHECK in our builders, and come back for a proper fix.
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
The unmap is called conditionally in the destructor, so unless one of the followings is true,
1. destructor is running in the same thread as unmap
2. destructor and unmap are cross-thread protected by the caller
3. the unmap is not necessary in the destructor / CHECK(!mapping_) in line 33
using two sequence-checkers would only prove the assumption without solving the issue.
We know 1 is not true. 2 and 3 need extra investigation.
I am very happy to solve the underlying issue, but it definitely needs more deep investigation, and it's the reason I am moving forward with a simple https://crrev.com/c/6965663 to unblock the enablement of DCHECKs first.
Related details
- Wez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wez (Gerrit)
Wez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Wez
| Code-Review | +1 |
2 comments
LGTM w/ the DO NOT SUBMIT bit not submitted.
base::AutoLock lock(lock_);- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Message from Zijie He![]( "Open in Gerrit")
- Wez
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 5 comments![]( "Open in Gerrit")
Votes added by Zijie He
| Commit-Queue | +1 |
5 comments
FYI, you can just hack the default for the arg in-place, leaving the `declare_args()` otherwise intact, for tests like this, in future 😊
This change is unnecessary after https://crrev.com/i/8394026 🎉
(The problem of keeping it as declare_args is that the infra/config is very complicated, and it's hard to tell if any GN args are overridden.)
// Functions in this class need to be thread-safe, they can be called from
// different threads.Is this true, or is it only the case that the `Map()` and `Unmap()` (and `GetNumberOfPlanes()` etc?) operations may be called on a different sequence from the setup/teardown sequence?
It's a good point, I will continue the investigation.
But also, even we are talking about the interface ClientNativePixmap, it can be used by different callers, and some of them may abuse it.
I believe we need to also add sequence checkers in the ui/gfx/linux/client_native_pixmap_dmabuf.cc to see if the same code path would be triggered on linux.
nit: This is only working via the `handle_`, which is `const`, so presumably no need for locking here?
Done
base::AutoLock lock(lock_);`handle_` is `const`, so no need to lock here?
Ah, yes, I think I messed two different intentions altogether; the const was also added in this CL. So yes, locking in this function is not necessary.
Done
base::AutoLock lock(lock_);Zijie HeNo need for lock because `handle_` is `const`.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |