New issue
Advanced search Search tips

Issue 822984 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 869154
issue 890051



Sign in to add a comment

DCHECK(result) in ProcessMetrics::ProcessMetrics

Project Member Reported by sorin@chromium.org, Mar 17 2018

Issue description

I hit this DCHECK in a local build of Chrome at commit hash 9fc3edbbf2f1.

It seems that the author of the code is not with Chrome anymore.

ProcessMetrics::ProcessMetrics(ProcessHandle process) : last_system_time_(0) {
  if (process) {
    HANDLE duplicate_handle;
    BOOL result = ::DuplicateHandle(::GetCurrentProcess(), process,
                                    ::GetCurrentProcess(), &duplicate_handle,
                                    PROCESS_QUERY_INFORMATION, FALSE, 0);
    DCHECK(result);
    process_.Set(duplicate_handle);
  }
}

The last error is 6, which indicates an invalid handle in the function call. I think there are some other problems with the code above. |duplicate_handle| is not initialized, the ::DuplicateHandle call can fail for any reason so the DCHECK could randomly fire, then |duplicate_handle| may contain garbage, and I did not look what the behavior of the surrounding code is when this handle is null because the duplication fail.


 	base.dll!base::debug::BreakDebugger() Line 21	C++
 	base.dll!logging::LogMessage::~LogMessage() Line 857	C++
>	base.dll!base::ProcessMetrics::ProcessMetrics(process) Line 298	C++
 	base.dll!base::ProcessMetrics::CreateProcessMetrics(process) Line 46	C++
 	chrome.dll!performance_monitor::ProcessMetricsHistory::Initialize(process_data, initial_update_sequence) Line 53	C++
 	chrome.dll!performance_monitor::PerformanceMonitor::MarkProcessAsAlive(process_data, current_update_sequence) Line 134	C++
 	chrome.dll!performance_monitor::PerformanceMonitor::MarkProcessesAsAliveOnUIThread(process_data_list, current_update_sequence) Line 179	C++
 	chrome.dll!base::internal::FunctorTraits<void (performance_monitor::PerformanceMonitor::*)(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >, int),void>::Invoke<performance_monitor::PerformanceMonitor *,std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >,int>(method, receiver_ptr, args, args) Line 447	C++
 	chrome.dll!base::internal::InvokeHelper<0,void>::MakeItSo<void (performance_monitor::PerformanceMonitor::*)(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >, int),performance_monitor::PerformanceMonitor *,std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >,int>(functor, args, args, args) Line 530	C++
 	chrome.dll!base::internal::Invoker<base::internal::BindState<void (performance_monitor::PerformanceMonitor::*)(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >, int),base::internal::UnretainedWrapper<performance_monitor::PerformanceMonitor>,std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >,int>,void ()>::RunImpl<void (performance_monitor::PerformanceMonitor::*)(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >, int),std::tuple<base::internal::UnretainedWrapper<performance_monitor::PerformanceMonitor>,std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >,int>,0,1,2>(functor, bound, ) Line 604	C++
 	chrome.dll!base::internal::Invoker<base::internal::BindState<void (performance_monitor::PerformanceMonitor::*)(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >, int),base::internal::UnretainedWrapper<performance_monitor::PerformanceMonitor>,std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > >,int>,void ()>::RunOnce(base) Line 572	C++
 	base.dll!base::OnceCallback<void ()>::Run() Line 95	C++
 	base.dll!base::debug::TaskAnnotator::RunTask(queue_function, pending_task) Line 61	C++
 	base.dll!base::internal::IncomingTaskQueue::RunTask(pending_task) Line 124	C++
 	base.dll!base::MessageLoop::RunTask(pending_task) Line 395	C++
 	base.dll!base::MessageLoop::DeferOrRunPendingTask(pending_task) Line 410	C++
 	base.dll!base::MessageLoop::DoWork() Line 451	C++
 	base.dll!base::MessagePumpForUI::DoRunLoop() Line 173	C++
 	base.dll!base::MessagePumpWin::Run(delegate) Line 58	C++
 	base.dll!base::MessageLoop::Run(application_tasks_allowed) Line 348	C++
 	base.dll!base::RunLoop::Run() Line 136	C++
 	chrome.dll!ChromeBrowserMainParts::MainMessageLoopRun(result_code) Line 2176	C++
 	content.dll!content::BrowserMainLoop::RunMainMessageLoopParts() Line 1057	C++
 	content.dll!content::BrowserMainRunnerImpl::Run() Line 161	C++
 	content.dll!content::BrowserMain(parameters) Line 46	C++
 	content.dll!content::RunNamedProcessTypeMain(process_type, main_function_params, delegate) Line 423	C++
 	content.dll!content::ContentMainRunnerImpl::Run() Line 703	C++
 	content.dll!content::ContentServiceManagerMainDelegate::RunEmbedderProcess() Line 51	C++
 	embedder.dll!service_manager::Main(params) Line 453	C++
 	content.dll!content::ContentMain(params) Line 19	C++
 	chrome.dll!ChromeMain(instance, sandbox_info, exe_entry_point_ticks) Line 101	C++
 	chrome.exe!MainDllLoader::Launch(instance, exe_entry_point_ticks) Line 198	C++
 	chrome.exe!wWinMain(instance, prev, , ) Line 230	C++
 	chrome.exe!invoke_main() Line 123	C++
 	chrome.exe!__scrt_common_main_seh() Line 283	C++
 	chrome.exe!__scrt_common_main() Line 326	C++
 	chrome.exe!wWinMainCRTStartup() Line 17	C++
 	kernel32.dll!00007ffed6438364()	Unknown
 	ntdll.dll!00007ffed8197091()	Unknown

 
Pending "fix" is in crrev.com/c/975405

Note that this developer found it separately and said:

I've found this while investigating crash dumps of our Chromium-based browser on CHECK(false) at //base/win/scoped_handle_verifier.cc:73.

Maybe there should be PCHECK() instead of DPCHECK() here? This might help to investigate why DuplicateHandle() fails in the first place and browser is going to CHECK() and crash anyway.


The fact that it showed up twice suggests that something has changed recently to trigger this. The fact that the error code is invalid-handle suggests a bug earlier that should be investigated.

Issue 821453 seems to be related to this one (probably the same).
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3a883075f699e67ab47d9991ac55b143017cfc3

commit e3a883075f699e67ab47d9991ac55b143017cfc3
Author: Stanislav Albreht <fatalerr@yandex-team.ru>
Date: Fri Mar 23 05:50:34 2018

Fix ProcessMetrics handle duplication

Assigning the uninitialized value to process_ when DuplicateHandle fails
leads to CHECK() later in //base/win/scoped_handle_verifier.cc.

Bug: 822984
Change-Id: Iafa6a6b6423c1374dc55c48d8fbfea60df288aec
Reviewed-on: https://chromium-review.googlesource.com/975405
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Stanislav Albreht <fatalerr@yandex-team.ru>
Cr-Commit-Position: refs/heads/master@{#545374}
[modify] https://crrev.com/e3a883075f699e67ab47d9991ac55b143017cfc3/base/process/process_metrics_win.cc

Status: Assigned (was: Untriaged)
This bug has an owner, thus, it's been triaged. Changing status to "assigned".
Blocking: 869154
Cc: davidbienvenu@chromium.org chengx@chromium.org
This is another handle race condition bug. The change mentioned in comment 3 doesn't actually fix the root cause, it just makes the ProcessMetrics constructor handle the bad handle more cleanly so that we don't propagate garbage.
Blocking: 890051
Cc: -davidbienvenu@chromium.org brucedaw...@chromium.org
Owner: davidbienvenu@chromium.org
Cc: -chengx@chromium.org

Sign in to add a comment