Invalid Handle Used to Update Metrics |
||||
Issue description
=======================================
VERIFIER STOP 0000000000000305: pid 0x3B88: Incorrect object type for handle.
000000000000050C : Handle value.
000000AEAD0FEDA8 : Object type name.
= "File"
00007FFB85FE6220 : Expected object type name. Use du to display it
= "Process"
0000000000000000 : Not used.
0:017> !handle 50C f
Handle 50c
Type File
Attributes 0
GrantedAccess 0x12019f:
ReadControl,Synch
Read/List,Write/Add,Append/SubDir/CreatePipe,ReadEA,WriteEA,ReadAttr,WriteAttr
HandleCount 2
PointerCount 65538
No Object Specific Information available
(3b88.29dc): Break instruction exception - code 80000003 (first chance)
vrfcore!VerifierStopMessageEx+0x6f9:
00007ffb`861a2149 cc int 3
0:017> kn20
# Child-SP RetAddr Call Site
00 000000ae`ad0fe970 00007ffb`85fcf4eb vrfcore!VerifierStopMessageEx+0x6f9
01 000000ae`ad0fecf0 00007ffb`85fcf569 vfbasics!AVrfpCheckObjectType+0xdf
02 000000ae`ad0fede0 00007ffb`85fd06a6 vfbasics!AVrfpHandleSanityChecks+0x45
03 000000ae`ad0fee30 00007ffb`945d3945 vfbasics!AVrfpNtQueryInformationProcess+0x46
04 000000ae`ad0fee70 00007ffb`713d36bb KERNELBASE!GetProcessTimes+0x45
05 000000ae`ad0fef00 00007ffb`724c4a55 chrome_7ffb70980000!base::ProcessMetrics::GetCPUUsage+0x2b [c:\b\build\slave\win64-pgo\build\src\base\process\process_metrics_win.cc @ 290]
06 (Inline Function) --------`-------- chrome_7ffb70980000!base::ProcessMetrics::GetPlatformIndependentCPUUsage+0x8 [c:\b\build\slave\win64-pgo\build\src\base\process\process_metrics.cc @ 59]
07 (Inline Function) --------`-------- chrome_7ffb70980000!performance_monitor::ProcessMetricsHistory::SampleMetrics+0xc [c:\b\build\slave\win64-pgo\build\src\chrome\browser\performance_monitor\process_metrics_history.cc @ 82]
08 000000ae`ad0fef60 00007ffb`70b6b460 chrome_7ffb70980000!performance_monitor::PerformanceMonitor::UpdateMetricsOnIOThread+0x95 [c:\b\build\slave\win64-pgo\build\src\chrome\browser\performance_monitor\performance_monitor.cc @ 199]
09 000000ae`ad0feff0 00007ffb`713fb793 chrome_7ffb70980000!base::internal::RunMixin<base::Callback<void __cdecl(void),0,0> >::Run+0x24 [c:\b\build\slave\win64-pgo\build\src\base\callback.h @ 68]
0a 000000ae`ad0ff020 00007ffb`713aa896 chrome_7ffb70980000!base::debug::TaskAnnotator::RunTask+0x183 [c:\b\build\slave\win64-pgo\build\src\base\debug\task_annotator.cc @ 61]
0:017> ?? this
class base::ProcessMetrics * 0x000001ee`3c68bac0
+0x000 process_ : 0x00000000`0000050c Void <--------
+0x008 processor_count_ : 0n48
+0x010 last_cpu_time_ : base::TimeTicks
+0x018 last_system_time_ : 0n172526
FIX:
Ensure that the handle received by ProcessMetrics is a process handle or requery for it via GetCurrentProcess()
,
Mar 23 2017
As I understand the code we have a list of Chrome process handles but we have not addrefed (called DuplicateHandle) them. Therefore there is an inherent race condition which is actually documented in the code:
if (!GetProcessTimes(process_, &creation_time, &exit_time,
&kernel_time, &user_time)) {
// We don't assert here because in some cases (such as in the Task Manager)
// we may call this function on a process that has just exited but we have
// not yet received the notification.
return 0;
}
Is this worth fixing? We'd have to DuplicateHandle as we add to the Task Manager's list of processes to scan, and then CloseHandle on the way out, with appropriate testing to make sure we don't create a handle leak.
This code dates back at least four years but it's hard to trace it past its last move in crrev.com/15806006. Adding stanisc@ in case he has any thoughts.
,
Mar 23 2017
We're on a hunt for handles at the moment, so anything that triggers a handle exception does make the investigation a bit harder. While the impact of this is low (yep, the API call will simply fail), it does add to the noise.
,
Mar 27 2017
It looks like this code is invoked by PerformanceMonitor rather than TaskManager (although it might be called by TaskManager as well). From the comments: // PerformanceMonitor is a tool which periodically monitors performance metrics // for histogram logging and possibly taking action upon noticing serious // performance degradation. May be calling DuplicateHandle is not a bad idea considering that that needs to be done only when an instance of ProcessMetrics is created rather than on every GetCPUUsage. I am Windows Sheriff this week. I think I could use that as an opportunity to fix this bug.
,
Mar 27 2017
> I am Windows Sheriff this week. I think I could use that as an opportunity to fix this bug. You are? I thought I was. But don't let that stop you from fixing this anyway. :)
,
Mar 27 2017
Yes, apparently I am not :) Somehow I looked at the calendar and thought it was me this week, probably because it starts with 's' :)
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e73f1a4f6e871894fc00f6129b4f1bc0847580cf commit e73f1a4f6e871894fc00f6129b4f1bc0847580cf Author: stanisc <stanisc@chromium.org> Date: Fri Mar 31 21:42:22 2017 Duplicate process handle in process_metrics_win.cc This is needed to avoid using an invalid handle to update metrics when there is a race condition with the process terminating. BUG= 702339 Review-Url: https://codereview.chromium.org/2779413002 Cr-Commit-Position: refs/heads/master@{#461237} [modify] https://crrev.com/e73f1a4f6e871894fc00f6129b4f1bc0847580cf/base/process/process_metrics.h [modify] https://crrev.com/e73f1a4f6e871894fc00f6129b4f1bc0847580cf/base/process/process_metrics_win.cc
,
Mar 31 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by robliao@chromium.org
, Mar 16 2017