Skip to content

Commit

Permalink
Reland "lacros: Check user dir is not accessed pre-login"
Browse files Browse the repository at this point in the history
This is a reland of commit c444bc1

The original change was causing a compilation error in some builds,
as the argument inside the DCHECKs refers to IsInitializedUserDataDir,
which is not declared/defined in non-debug builds.

This CL changes:
- The complex !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) condition
  to the more readable DCHECK_IS_ON().
- Wraps the DCHECKs into `#if DCHECK_IS_ON()` blocks to avoid
  the previously mentioned compilation errors.

Original change's description:
> lacros: Check user dir is not accessed pre-login
>
> As part of the effort to execute more operations at login screen
> before blocking waiting for the user to login, we want to remove
> accesses to the cryptohome that happen early in the initialization
> process.
>
> This change ensures (via DCHECKs) that any access to the cryptohome
> happens after user dir initialization when prelaunching.
>
> BUG=1495212
> TEST=CQ
> tast run ${DUT} lacros.Basic
> tast run ${DUT} lacros.LoginScreenLaunch.rootfs
>
> Change-Id: Id6df5e28216beb8058e8349ca849bf53e9f46daf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4944753
> Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Commit-Queue: Andrea Orru <andreaorru@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1214708}

BUG=1495212

Change-Id: Ie34ff8e25a551b33ed9b3d7b7e825ef6bdf70f3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4975550
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Andrea Orru <andreaorru@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216769}
  • Loading branch information
Andrea Orru authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent eb0d895 commit 3bdd67f
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 1 deletion.
9 changes: 8 additions & 1 deletion chrome/app/chrome_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,12 @@ struct MainFunction {

// Initializes the user data dir. Must be called before InitializeLocalState().
void InitializeUserDataDir(base::CommandLine* command_line) {
#if BUILDFLAG(IS_CHROMEOS_LACROS) && DCHECK_IS_ON()
// In debug builds of Lacros, we keep track of when the user data dir
// is initialized, to ensure the cryptohome is not accessed before login
// when prelaunching at login screen.
chromeos::lacros_paths::SetInitializedUserDataDir();
#endif
#if BUILDFLAG(IS_WIN)
// Reach out to chrome_elf for the truth on the user data directory.
// Note that in tests, this links to chrome_elf_test_stubs.
Expand Down Expand Up @@ -1425,13 +1431,14 @@ void ChromeMainDelegate::PreSandboxStartup() {
base::CPU cpu_info;
#endif

// Initialize the user data dir for any process type that needs it.
bool initialize_user_data_dir = chrome::ProcessNeedsProfileDir(process_type);
#if BUILDFLAG(IS_CHROMEOS_LACROS)
// In Lacros, when prelaunching at login screen, we postpone the
// initialization of the user data directory.
// We verify that no access happens before login via DCHECKs.
initialize_user_data_dir &= !chromeos::IsLaunchedWithPostLoginParams();
#endif
// Initialize the user data dir for any process type that needs it.
if (initialize_user_data_dir) {
InitializeUserDataDir(base::CommandLine::ForCurrentProcess());
}
Expand Down
1 change: 1 addition & 0 deletions chrome/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,7 @@ static_library("constants") {
deps += [
"//chromeos/crosapi/cpp:crosapi_constants",
"//chromeos/lacros:lacros_paths",
"//chromeos/startup",
]
public_deps += [ "//chromeos/crosapi/mojom" ]
} else if (is_linux || is_chromeos) {
Expand Down
1 change: 1 addition & 0 deletions chrome/common/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ include_rules = [
"+chrome/services/file_util/public",
"+chromeos/crosapi",
"+chromeos/lacros/lacros_paths.h",
"+chromeos/startup/startup.h",
"+components/autofill/content/common",
"+components/autofill/core/common",
"+components/bookmarks/common",
Expand Down
7 changes: 7 additions & 0 deletions chrome/common/chrome_paths.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include "chrome/common/chrome_switches.h"
#include "chromeos/crosapi/cpp/crosapi_constants.h" // nogncheck
#include "chromeos/lacros/lacros_paths.h"
#include "chromeos/startup/startup.h" // nogncheck
#endif

namespace {
Expand Down Expand Up @@ -186,6 +187,12 @@ bool PathProvider(int key, base::FilePath* result) {
base::FilePath cur;
switch (key) {
case chrome::DIR_USER_DATA:
#if BUILDFLAG(IS_CHROMEOS_LACROS) && DCHECK_IS_ON()
// Check that the user data directory is not accessed before
// initialization when prelaunching at login screen.
DCHECK(chromeos::lacros_paths::IsInitializedUserDataDir() ||
!chromeos::IsLaunchedWithPostLoginParams());
#endif
if (!GetDefaultUserDataDirectory(&cur)) {
return false;
}
Expand Down
21 changes: 21 additions & 0 deletions chromeos/lacros/lacros_paths.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ struct LacrosPaths {
base::FilePath ash_resource_dir;
};

#if DCHECK_IS_ON()
// Keep track of whether the user data directory was initialized.
bool g_initialized_user_data_dir = false;
#endif

LacrosPaths& GetLacrosPaths() {
static base::NoDestructor<LacrosPaths> lacros_paths;
return *lacros_paths;
Expand All @@ -35,6 +40,11 @@ bool PathProvider(int key, base::FilePath* result) {
}
return true;
case chromeos::lacros_paths::USER_DATA_DIR:
#if DCHECK_IS_ON()
// Check that the user data directory is not accessed before
// initialization.
DCHECK(chromeos::lacros_paths::IsInitializedUserDataDir());
#endif
// The value for USER_DATA_DIR should be consistent with ash-side
// UserDataDir defined in browser_util::GetUserDataDir().
if (base::SysInfo::IsRunningOnChromeOS()) {
Expand All @@ -53,6 +63,17 @@ bool PathProvider(int key, base::FilePath* result) {
namespace chromeos {
namespace lacros_paths {

#if DCHECK_IS_ON()
bool IsInitializedUserDataDir() {
return g_initialized_user_data_dir;
}

void SetInitializedUserDataDir() {
DCHECK(!g_initialized_user_data_dir);
g_initialized_user_data_dir = true;
}
#endif

void RegisterPathProvider() {
base::PathService::RegisterProvider(PathProvider, PATH_START, PATH_END);
}
Expand Down
11 changes: 11 additions & 0 deletions chromeos/lacros/lacros_paths.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#ifndef CHROMEOS_LACROS_LACROS_PATHS_H_
#define CHROMEOS_LACROS_LACROS_PATHS_H_

#include "base/dcheck_is_on.h"

namespace base {
class FilePath;
}
Expand All @@ -30,6 +32,15 @@ enum {
PATH_END
};

#if DCHECK_IS_ON()
// Returns true if the user data directory has been initialized,
// false otherwise.
bool IsInitializedUserDataDir();

// Signals that the user data directory has been initialized.
void SetInitializedUserDataDir();
#endif

// Call once to register the provide for the path keys defined above.
void RegisterPathProvider();

Expand Down

0 comments on commit 3bdd67f

Please sign in to comment.