Fix use-after-close for process HANDLE in PerformOSMemoryDump [chromium/src : main]
Javier Flores Assad (Gerrit)
Javier Flores Assad voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Chris Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Fortiner (Gerrit)
Sam Fortiner added 2 comments![]( "Open in Gerrit")
Fix use-after-close for process HANDLE in PerformOSMemoryDump
There were two issues in the original code:
Real: base::Process::Open(pid) returns a temporary base::Process
(a move-only RAII owner of the handle). .Handle() just borrows the raw
handle. At the semicolon, that temporary base::Process is destroyed.
So the handle is dangling/closed before it's ever used.
Theoretical: The pid could be no longer valid by the time we open a
handle to it. This means that ::Open could fail. But, in practice, this
will not occur because the browser holds open handles to the processes
and closed on the same sequence that services the dump request.The current CL description doesn't appear to capture the full extent of the issue/fix. It discusses the lifetime bug but not the `kNullProcessId` special-casing which also seems important for Windows/Mac. Here's a suggested revision:
There were two issues in the original code:
First, base::Process::Open(pid) returns a temporary base::Process (a
move-only RAII owner of the handle). .Handle() just borrows the raw
handle, which is closed when the temporary is destroyed at the
semicolon leaving FillOSMemoryDump/FillProcessMemoryMaps with an
already-closed handle.
Second, pids can legitimately contain kNullProcessId, meaning "dump my
own process" (common on non-Linux/ChromeOS, see RequestOSMemoryDump's
mojom comment and QueuedRequestDispatcher). Be aware that
base::Process::Open(kNullProcessId) is invalid on every platform, so
this CL special-cases it to base::Process::Current(). Without this, the
new IsValid() check added below would regress self-dumps everywhere
(they already silently failed on Windows).
Note: pid-not-found failures now correctly report kProcessNotFound
instead of kFillOsMemoryDumpFailed, shifting counts between these two
UMA buckets. No other behavior change.
void ClientProcessImpl::PerformOSMemoryDump(OSMemoryDumpArgs args) {This seems like a legitimate test gap. Can we add a new client_process_impl_unittest.cc that calls RequestOSMemoryDump on a real ClientProcessImpl for two cases: `pids = {base::kNullProcessId}` (the "dump my own process" used on non-Linux/ChromeOS) and `pids = {base::Process::Current().Pid()}` (the real-pid path Linux uses), asserting `outcome == kSuccess` and that the returned dump has sane, non-default fields (e.g. non-zero resident set). Use `mojom::MemoryMapOption::NONE` with no extra `MemDumpFlags` to keep it to real but cheap syscalls (avoid MODULES/FULL).
Related details
- Chris Hamilton
- Javier Flores Assad
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. |
Javier Flores Assad (Gerrit)
Javier Flores Assad added 2 comments![]( "Open in Gerrit")
Fixed description, will add test.
Fix use-after-close for process HANDLE in PerformOSMemoryDump
There were two issues in the original code:
Real: base::Process::Open(pid) returns a temporary base::Process
(a move-only RAII owner of the handle). .Handle() just borrows the raw
handle. At the semicolon, that temporary base::Process is destroyed.
So the handle is dangling/closed before it's ever used.
Theoretical: The pid could be no longer valid by the time we open a
handle to it. This means that ::Open could fail. But, in practice, this
will not occur because the browser holds open handles to the processes
and closed on the same sequence that services the dump request.The current CL description doesn't appear to capture the full extent of the issue/fix. It discusses the lifetime bug but not the `kNullProcessId` special-casing which also seems important for Windows/Mac. Here's a suggested revision:
There were two issues in the original code:
First, base::Process::Open(pid) returns a temporary base::Process (a
move-only RAII owner of the handle). .Handle() just borrows the raw
handle, which is closed when the temporary is destroyed at the
semicolon leaving FillOSMemoryDump/FillProcessMemoryMaps with an
already-closed handle.Second, pids can legitimately contain kNullProcessId, meaning "dump my
own process" (common on non-Linux/ChromeOS, see RequestOSMemoryDump's
mojom comment and QueuedRequestDispatcher). Be aware that
base::Process::Open(kNullProcessId) is invalid on every platform, so
this CL special-cases it to base::Process::Current(). Without this, the
new IsValid() check added below would regress self-dumps everywhere
(they already silently failed on Windows).Note: pid-not-found failures now correctly report kProcessNotFound
instead of kFillOsMemoryDumpFailed, shifting counts between these two
UMA buckets. No other behavior change.
- Chris Hamilton
- Sam Fortiner
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. |
Javier Flores Assad (Gerrit)
Javier Flores Assad added 2 comments![]( "Open in Gerrit")
Thanks for your suggestion. Specific tests added.
void ClientProcessImpl::PerformOSMemoryDump(OSMemoryDumpArgs args) {This seems like a legitimate test gap. Can we add a new client_process_impl_unittest.cc that calls RequestOSMemoryDump on a real ClientProcessImpl for two cases: `pids = {base::kNullProcessId}` (the "dump my own process" used on non-Linux/ChromeOS) and `pids = {base::Process::Current().Pid()}` (the real-pid path Linux uses), asserting `outcome == kSuccess` and that the returned dump has sane, non-default fields (e.g. non-zero resident set). Use `mojom::MemoryMapOption::NONE` with no extra `MemDumpFlags` to keep it to real but cheap syscalls (avoid MODULES/FULL).
Done
Related details
- Chris Hamilton
- Sam Fortiner
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Fortiner (Gerrit)
Sam Fortiner voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sam Fortiner
| Code-Review | +1 |
3 comments
Thanks for adding the test!
mojom::RequestOutcome RequestOSMemoryDumpAndWait(Not blocking for me, but I noticed that `MemoryTracingIntegrationTest` has similar tests. chrisha@ may have an opinion about if we should add these tests here, in this new test fixture, vs add a test to `MemoryTracingIntegrationTest` instead that covers the same things.
auto result = future.Take();
if (dumps) {
*dumps = std::move(std::get<1>(result));
}
return std::get<0>(result);This can be rewritten as follows I believe:
```suggestion
auto result = future.Take();
if (dumps) {
*dumps = std::move(std::get<RawOSMemDumpMap>(result));
}
return std::get<mojom::RequestOutcome>(result);
```
Related details
- Chris Hamilton
- Javier Flores Assad
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. |
Javier Flores Assad (Gerrit)
Javier Flores Assad added 3 comments![]( "Open in Gerrit")
Decided to keep the UTs in separate files, reason on reply to comment.
Changed the getters to use types instead of indexes (good call)
Not blocking for me, but I noticed that `MemoryTracingIntegrationTest` has similar tests. chrisha@ may have an opinion about if we should add these tests here, in this new test fixture, vs add a test to `MemoryTracingIntegrationTest` instead that covers the same things.
Similar, but I prefer to have UTs match the file/class they test.
• tracing_integration_unittest.cc — MemoryTracingIntegrationTest , uses a MockCoordinator , and tests the memory-infra tracing path: RequestChromeMemoryDump , periodic dumps, trace configs, allowlisting. It does not test RequestOSMemoryDump at all.
• client_process_impl_unittest.cc ClientProcessImplTest , no coordinator, tests the OS memory dump path ( RequestOSMemoryDump ) against a real process. The only overlap with the existing file is SetUp boilerplate (tracing env + MDM), not the behavior under test.
I recommend we keep them separate.
auto result = future.Take();
if (dumps) {
*dumps = std::move(std::get<1>(result));
}
return std::get<0>(result);This can be rewritten as follows I believe:
```suggestion
auto result = future.Take();
if (dumps) {
*dumps = std::move(std::get<RawOSMemDumpMap>(result));
}
return std::get<mojom::RequestOutcome>(result);
```
- Chris Hamilton
- Sam Fortiner
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Fortiner (Gerrit)
Sam Fortiner voted Code-Review+1![]( "Open in Gerrit")
- Chris Hamilton
- Javier Flores Assad
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 7 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
7 comments
Thanks a lot for the fix!
using RawOSMemDumpMap = base::flat_map<base::ProcessId, mojom::RawOSMemDumpPtr>;Optional nit: the current advice in https://chromium.googlesource.com/chromium/src/+/main/base/containers/README.md says to use `absl::flat_hash_map` by default because it has better general performance characteristics.
Not a big deal for the test, just evangelizing.
EDIT: nevermind, I see that's an alias for an existing map type. I'll leave the evangelizing here since I spent all that time on it.
base::SingleThreadTaskRunner::GetCurrentDefault());Nit: prefer `task_environment_.GetMainThreadTaskRunner()`. It's cleaner.
mojo::PendingRemote<mojom::ClientProcess> process;Nit: this could use a comment saying we don't need the PendingRemote because we call the overridden methods on `client_process_` directly. I was a bit confused about why it was allowed to go out of scope until I realized that.
client_process_.reset(new ClientProcessImpl(Nit: prefer `child_process_ = base::WrapUnique(new ...)`, which is a keyword that can more easily be audited for misuse.
client_process_.reset();Optional nit: https://google.github.io/googletest/faq.html#CtorVsSetUp recommends only using SetUp / TearDown if there's setup code that can't run in the constructor / destructor. (eg. calls to ASSERT_xx). In this case if you use the constructor instead of SetUp(), the destructor wouldn't have to remember to clear any of these, it can just call `ResetForTesting`.
(And I believe TracingEnvironment wouldn't need to be a unique_ptr, since you wouldn't need to reset it before the destructor.)
raw_ptr<base::trace_event::MemoryDumpManager> mdm_ = nullptr;Optional nit: this isn't needed. You could just call `GetInstance()->ResetForTesting()` in teardown.
Related details
- Chris Hamilton
- Javier Flores Assad
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. |
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
mojom::RequestOutcome RequestOSMemoryDumpAndWait(Javier Flores AssadNot blocking for me, but I noticed that `MemoryTracingIntegrationTest` has similar tests. chrisha@ may have an opinion about if we should add these tests here, in this new test fixture, vs add a test to `MemoryTracingIntegrationTest` instead that covers the same things.
Similar, but I prefer to have UTs match the file/class they test.
• tracing_integration_unittest.cc — MemoryTracingIntegrationTest , uses a MockCoordinator , and tests the memory-infra tracing path: RequestChromeMemoryDump , periodic dumps, trace configs, allowlisting. It does not test RequestOSMemoryDump at all.
• client_process_impl_unittest.cc ClientProcessImplTest , no coordinator, tests the OS memory dump path ( RequestOSMemoryDump ) against a real process. The only overlap with the existing file is SetUp boilerplate (tracing env + MDM), not the behavior under test.I recommend we keep them separate.
Agreed.
chrisha@ no longer works on Chrome, BTW. (He's still at Google but moved to a different project several years ago.)
Javier Flores Assad (Gerrit)
Javier Flores Assad added 6 comments![]( "Open in Gerrit")
Thank you for the review Joe, comments addressed.
Nit: prefer `task_environment_.GetMainThreadTaskRunner()`. It's cleaner.
Done
Nit: this could use a comment saying we don't need the PendingRemote because we call the overridden methods on `client_process_` directly. I was a bit confused about why it was allowed to go out of scope until I realized that.
Done
Nit: prefer `child_process_ = base::WrapUnique(new ...)`, which is a keyword that can more easily be audited for misuse.
Done
client_process_.reset();Optional nit: https://google.github.io/googletest/faq.html#CtorVsSetUp recommends only using SetUp / TearDown if there's setup code that can't run in the constructor / destructor. (eg. calls to ASSERT_xx). In this case if you use the constructor instead of SetUp(), the destructor wouldn't have to remember to clear any of these, it can just call `ResetForTesting`.
(And I believe TracingEnvironment wouldn't need to be a unique_ptr, since you wouldn't need to reset it before the destructor.)
Done
raw_ptr<base::trace_event::MemoryDumpManager> mdm_ = nullptr;Optional nit: this isn't needed. You could just call `GetInstance()->ResetForTesting()` in teardown.
- Chris Hamilton
- Joe Mason
- Sam Fortiner
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Fortiner (Gerrit)
Sam Fortiner voted Code-Review+1![]( "Open in Gerrit")
- Chris Hamilton
- Javier Flores Assad
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Flores Assad (Gerrit)
Javier Flores Assad added 1 comment![]( "Open in Gerrit")
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted Code-Review+1![]( "Open in Gerrit")
- Javier Flores Assad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Flores Assad (Gerrit)
Javier Flores Assad 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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix use-after-close for process HANDLE in PerformOSMemoryDump
There were two issues in the original code:
First, base::Process::Open(pid) returns a temporary base::Process (a
move-only RAII owner of the handle). .Handle() just borrows the raw
handle, which is closed when the temporary is destroyed at the semicolon
leaving FillOSMemoryDump/FillProcessMemoryMaps with an already-closed
handle.
Second, pids can legitimately contain kNullProcessId, meaning "dump my
own process" (common on non-Linux/ChromeOS, see RequestOSMemoryDump's
mojom comment and QueuedRequestDispatcher). Be aware that
base::Process::Open(kNullProcessId) is invalid on every platform, so
this CL special-cases it to base::Process::Current(). Without this, the
new IsValid() check added below would regress self-dumps everywhere
(they already silently failed on Windows).
Note: pid-not-found failures now correctly report kProcessNotFound
instead of kFillOsMemoryDumpFailed, shifting counts between these two
UMA buckets. No other behavior change.
- M services/resource_coordinator/BUILD.gn
- M services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
- A services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl_unittest.cc
Code-Review: +1 by Joe Mason, +1 by Sam Fortiner
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |