Try NtDuplicateObject instead of DuplicateHandle on Windows 8/8.1 for debugging AppContainer relate… (issue 1101913002 by shrikant@chromium.org)

[shri...@chromium.org]

Reviewers: cpu, jschuh (out April 2015),

Message:
ptal.

Description:
Try NtDuplicateObject instead of DuplicateHandle on Windows 8/8.1 for
debugging
AppContainer related failures.
This patch should be reverted as soon as we get some confirmation that
NtDuplicateObject is working better compared to DuplicateHandle.
Theory behind this patch is that it might be possible that on machines which
reporting this find of failure have some software like AV which might be
intercepting calls to DuplciateHandle and may be failing somewhere in
intercepted code path due to AppContainer.
Looking in IDA error code that we are receiving 0xC0000023 (Which translates
INSUFFICIENT_BUFFER (0x7a) in win32 language) doesn't seem to be in
NtDuplicateObject code path (At least not found easily.).

BUG=468922
R=jsc...@chromium.org,c...@chromium.org

Please review this at https://codereview.chromium.org/1101913002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+45, -5 lines):
M base/win/scoped_process_information.cc


Index: base/win/scoped_process_information.cc
diff --git a/base/win/scoped_process_information.cc
b/base/win/scoped_process_information.cc
index
bb2463774a8a326ffb769d7aadf6852bf53e802f..82f45de062e8c03c1036beba8af8213d2998380e
100644
--- a/base/win/scoped_process_information.cc
+++ b/base/win/scoped_process_information.cc
@@ -6,6 +6,7 @@

#include "base/logging.h"
#include "base/win/scoped_handle.h"
+#include "base/win/windows_version.h"

namespace base {
namespace win {
@@ -20,11 +21,50 @@ bool CheckAndDuplicateHandle(HANDLE source,
ScopedHandle* target) {
return true;

HANDLE temp = NULL;
- if (!::DuplicateHandle(::GetCurrentProcess(), source,
- ::GetCurrentProcess(), &temp, 0, FALSE,
- DUPLICATE_SAME_ACCESS)) {
- DPLOG(ERROR) << "Failed to duplicate a handle.";
- return false;
+
+ // TODO(shrikant): Remove following code as soon as we gather some
+ // information regarding AppContainer related DuplicateHandle failures
that
+ // only seem to happen on certain machine and only random launches
(normally
+ // renderer launches seem to succeed even on those machines.)
+ if (base::win::GetVersion() == base::win::VERSION_WIN8 ||
+ base::win::GetVersion() == base::win::VERSION_WIN8_1) {
+ typedef LONG NTSTATUS;
+ typedef NTSTATUS (WINAPI *NtDuplicateObject)(
+ IN HANDLE SourceProcess,
+ IN HANDLE SourceHandle,
+ IN HANDLE TargetProcess,
+ OUT PHANDLE TargetHandle,
+ IN ACCESS_MASK DesiredAccess,
+ IN ULONG Attributes,
+ IN ULONG Options);
+
+ typedef ULONG (WINAPI *RtlNtStatusToDosError)(IN NTSTATUS Status);
+
+ NtDuplicateObject nt_duplicate_object =
+ reinterpret_cast<NtDuplicateObject>(::GetProcAddress(
+ GetModuleHandle(L"ntdll.dll"), "NtDuplicateObject"));
+ if (nt_duplicate_object != NULL) {
+ NTSTATUS status = nt_duplicate_object(::GetCurrentProcess(), source,
+ ::GetCurrentProcess(), &temp,
+ 0, FALSE,
DUPLICATE_SAME_ACCESS);
+ if (status < 0) {
+ DPLOG(ERROR) << "Failed to duplicate a handle.";
+ RtlNtStatusToDosError ntstatus_to_doserror =
+ reinterpret_cast<RtlNtStatusToDosError>(::GetProcAddress(
+ GetModuleHandle(L"ntdll.dll"), "RtlNtStatusToDosError"));
+ if (ntstatus_to_doserror != NULL) {
+ ::SetLastError(ntstatus_to_doserror(status));
+ }
+ return false;
+ }
+ }
+ } else {
+ if (!::DuplicateHandle(::GetCurrentProcess(), source,
+ ::GetCurrentProcess(), &temp, 0, FALSE,
+ DUPLICATE_SAME_ACCESS)) {
+ DPLOG(ERROR) << "Failed to duplicate a handle.";
+ return false;
+ }
}
target->Set(temp);
return true;

[c...@chromium.org]

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc
File base/win/scoped_process_information.cc (right):

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc#newcode31
base/win/scoped_process_information.cc:31: typedef LONG NTSTATUS;
just do LONG w/o typedef

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc#newcode52
base/win/scoped_process_information.cc:52: RtlNtStatusToDosError
ntstatus_to_doserror =
do we need this? I don't where?

https://codereview.chromium.org/1101913002/

[shri...@chromium.org]

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc
File base/win/scoped_process_information.cc (right):

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc#newcode31
base/win/scoped_process_information.cc:31: typedef LONG NTSTATUS;

On 2015/04/24 02:39:56, cpu wrote:
> just do LONG w/o typedef

Done.

https://codereview.chromium.org/1101913002/diff/1/base/win/scoped_process_information.cc#newcode52
base/win/scoped_process_information.cc:52: RtlNtStatusToDosError
ntstatus_to_doserror =

On 2015/04/24 02:39:56, cpu wrote:
> do we need this? I don't where?

kernelbase::DuplicateHandle calls NtDupliateObject and sets up last
error after receiving NTSTATUS. Tried to keep the same behavior. One
more reason is that in caller chain we report histogram value which
indicates GetLastError(), so during this experiment we can find out if
that value changes as well.

https://codereview.chromium.org/1101913002/

[c...@chromium.org]

lgtm



https://codereview.chromium.org/1101913002/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1101913002/20001


https://codereview.chromium.org/1101913002/

[commi...@chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/1101913002/

[commi...@chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/f7d692ca5448140fb21ffa42ca8b1535aae0b490
Cr-Commit-Position: refs/heads/master@{#326943}

https://codereview.chromium.org/1101913002/