Skip to content

Commit

Permalink
Reland "Use consistent DLL to detect Application Verifier"
Browse files Browse the repository at this point in the history
This reverts commit 956f829.

Reason for revert: The previous version of this change caused an access
violation when sandbox/win/src/target_interceptions.cc::
TargetNtMapViewOfSection() tried to access constant
kApplicationVerifierDllName, which was declared in base/win/win_util.h
and defined in win_util.cc.

This access violation happened because base.dll, from which the constant
was imported, is not yet resolved by the time TargetNtMapViewOfSection()
is called. This function, as well as the others in
target_interceptions.cc, can't reference any non-static variables or
functions in other modules besides those exposed via the g_nt structure.

To address this, the constant kApplicationVerifierDllName was moved into
the base_static build rule, whose source files are statically linked
into code that depends on them (rather than dynamically linked via
dllexport).

Original change's description:
> Revert "Use consistent DLL to detect Application Verifier"
>
> This reverts commit fd8fe93.
>
> Reason for revert: this change is causing chrome.exe to crash on
> startup. I received a report that chrome.exe looked for the string
> constant in memory and encountered some kind of error which caused it
> to crash. I will roll back for now to investigate.
>
> Original change's description:
> > Use consistent DLL to detect Application Verifier
> >
> > To detect whether Application Verifier is running,
> > sandbox/win/src/target_interceptions.cc checks if vrfcore.dll is loaded,
> > while sandbox/win/src/handle_closer_agent.cc checks for verifier.dll.
> >
> > This change makes both use verifier.dll, to be consistent and remove any
> > confusion around whether these two are checking for the same thing (they
> > are).
> >
> >       Open Application Verifier
> >       File > Add Application > chrome.exe (any chrome.exe binary works)
> >       Under 'Tests', uncheck everything but Basics > Handles, then Save
> >       Run chrome.exe with this change; it works normally
> >         (on versions without lines 176-177, no pages will load)
> >       Return to Application Verifier
> >       Right-click chrome.exe > Delete Application, then Save
> >         (so Chrome runs normally again)
> >
> > Test: Manual
> > Change-Id: If97a1115373c394f0cbbf1d31d3ca7b60549bbee
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776781
> > Commit-Queue: Jesse McKenna <jessemckenna@google.com>
> > Reviewed-by: Greg Thompson <grt@chromium.org>
> > Reviewed-by: Will Harris <wfh@chromium.org>
> > Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#700323}
>
> TBR=brucedawson@chromium.org,wfh@chromium.org,grt@chromium.org,jessemckenna@google.com
>
> Change-Id: I79f41f6e93813befaf4e49604f7dcc1f8c1ddb48
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1828234
> Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
> Commit-Queue: Jesse McKenna <jessemckenna@google.com>
> Cr-Commit-Position: refs/heads/master@{#700493}

Change-Id: I0c43a20433dbbb54ebdf8d1a2ec71e09adb8afa9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829544
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Jesse McKenna <jessemckenna@google.com>
Cr-Commit-Position: refs/heads/master@{#703898}
  • Loading branch information
jessemckenna authored and Commit Bot committed Oct 8, 2019
1 parent 1414b78 commit cc1b611
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 4 deletions.
5 changes: 5 additions & 0 deletions base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2167,6 +2167,11 @@ static_library("base_static") {
]

if (is_win) {
sources += [
"win/static_constants.cc",
"win/static_constants.h",
]

public_deps = [
"//base/win:pe_image",
]
Expand Down
13 changes: 13 additions & 0 deletions base/win/static_constants.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/win/static_constants.h"

namespace base {
namespace win {

const char kApplicationVerifierDllName[] = "verifier.dll";

} // namespace win
} // namespace base
21 changes: 21 additions & 0 deletions base/win/static_constants.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// Defines constants needed before imports (like base.dll) are fully resolved.
// For example, constants defined here can be used by interceptions (i.e. hooks)
// in the sandbox, which run before imports are resolved, and can therefore only
// reference static variables.

#ifndef BASE_WIN_STATIC_CONSTANTS_H_
#define BASE_WIN_STATIC_CONSTANTS_H_

namespace base {
namespace win {

extern const char kApplicationVerifierDllName[];

} // namespace win
} // namespace base

#endif // BASE_WIN_STATIC_CONSTANTS_H_
3 changes: 2 additions & 1 deletion sandbox/win/src/handle_closer_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <stddef.h>

#include "base/logging.h"
#include "base/win/static_constants.h"
#include "sandbox/win/src/nt_internals.h"
#include "sandbox/win/src/win_utils.h"

Expand Down Expand Up @@ -173,7 +174,7 @@ bool HandleCloserAgent::CloseHandles() {

// Skip closing these handles when Application Verifier is in use in order to
// avoid invalid-handle exceptions.
if (GetModuleHandleW(L"vrfcore.dll"))
if (GetModuleHandleA(base::win::kApplicationVerifierDllName))
return true;

// Set up buffers for the type info and the name.
Expand Down
7 changes: 4 additions & 3 deletions sandbox/win/src/target_interceptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "sandbox/win/src/target_interceptions.h"

#include "base/win/static_constants.h"
#include "sandbox/win/src/interception_agent.h"
#include "sandbox/win/src/sandbox_factory.h"
#include "sandbox/win/src/sandbox_nt_util.h"
Expand All @@ -12,7 +13,6 @@ namespace sandbox {

SANDBOX_INTERCEPT NtExports g_nt;

const char VERIFIER_DLL_NAME[] = "verifier.dll";
const char KERNEL32_DLL_NAME[] = "kernel32.dll";

enum SectionLoadState {
Expand Down Expand Up @@ -60,8 +60,9 @@ TargetNtMapViewOfSection(NtMapViewOfSectionFunction orig_MapViewOfSection,
// indicates Application Verifier is enabled and we should wait until
// the next module is loaded.
if (ansi_module_name &&
(g_nt._strnicmp(ansi_module_name, VERIFIER_DLL_NAME,
sizeof(VERIFIER_DLL_NAME)) == 0))
(g_nt._strnicmp(
ansi_module_name, base::win::kApplicationVerifierDllName,
g_nt.strlen(base::win::kApplicationVerifierDllName) + 1) == 0))
break;

if (ansi_module_name &&
Expand Down

0 comments on commit cc1b611

Please sign in to comment.