Ignore "--staging-dir" passed to odrefresh.

An upcoming change will make odrefresh use `rename` instead of copying
when commiting staging files. This means the staging files must be on
the same filesystem as the output dir. Therefore, we can't respect
"--staging-dir" as it may point to a location on a different filesystem.

Currently, the only user of "--staging-dir" is CompOS. In fact, we don't
need to stage files in CompOS. If the compilation fails (partially or
entirely), CompOS will not sign any artifacts, and odsign will discard
CompOS outputs entirely. After this change, if odrefresh is run in
CompOS, it writes to the output dir directly.

Test: adb shell /apex/com.android.compos/bin/composd_cmd test-compile
Test: -
  1. Install an ART apex.
  2. adb shell cmd jobscheduler run android 5132251
  3. See that CompOS finishes successfully.
  4. Reboot and see that odsign accepts CompOS'es outputs.
Test: atest odsign_e2e_tests_full
Test: atest art_standalone_odrefresh_tests
Test: atest ArtGtestsTargetChroot:OdRefreshTest
Change-Id: Icc4c6a378bc85e428003380c7d4014d9b2516c01

6 files changed, 127 insertions, 96 deletions

diff --git a/odrefresh/odr_config.h b/odrefresh/odr_config.h
index 0e937d49c1..e8f44e181a 100644
--- a/odrefresh/odr_config.h
+++ b/odrefresh/odr_config.h
@@ -143,10 +143,6 @@ class OdrConfig final {
   // A helper for reading from `system_properties_`.
   OdrSystemProperties odr_system_properties_;
 
-  // Staging directory for artifacts. The directory must exist and will be automatically removed
-  // after compilation. If empty, use the default directory.
-  std::string staging_dir_;
-
  public:
   explicit OdrConfig(const char* program_name)
       : dry_run_(false),
@@ -232,9 +228,6 @@ class OdrConfig final {
   const std::string& GetSystemServerCompilerFilter() const {
     return system_server_compiler_filter_;
   }
-  const std::string& GetStagingDir() const {
-    return staging_dir_;
-  }
   bool GetCompilationOsMode() const { return compilation_os_mode_; }
   bool GetMinimal() const { return minimal_; }
   const OdrSystemProperties& GetSystemProperties() const { return odr_system_properties_; }
@@ -276,10 +269,6 @@ class OdrConfig final {
 
   void SetBootClasspath(const std::string& classpath) { boot_classpath_ = classpath; }
 
-  void SetStagingDir(const std::string& staging_dir) {
-    staging_dir_ = staging_dir;
-  }
-
   const std::string& GetStandaloneSystemServerJars() const {
     return standalone_system_server_jars_;
   }
diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc
index f515aefa10..4b9688781b 100644
--- a/odrefresh/odrefresh.cc
+++ b/odrefresh/odrefresh.cc
@@ -101,6 +101,7 @@ using ::android::base::Dirname;
 using ::android::base::Join;
 using ::android::base::ParseInt;
 using ::android::base::Result;
+using ::android::base::ScopeGuard;
 using ::android::base::SetProperty;
 using ::android::base::Split;
 using ::android::base::StartsWith;
@@ -193,28 +194,6 @@ bool MoveOrEraseFiles(const std::vector<std::unique_ptr<File>>& files,
   return true;
 }
 
-Result<std::string> CreateStagingDirectory() {
-  std::string staging_dir = GetArtApexData() + "/staging";
-
-  std::error_code ec;
-  if (std::filesystem::exists(staging_dir, ec)) {
-    if (!std::filesystem::remove_all(staging_dir, ec)) {
-      return Errorf(
-          "Could not remove existing staging directory '{}': {}", staging_dir, ec.message());
-    }
-  }
-
-  if (mkdir(staging_dir.c_str(), S_IRWXU) != 0) {
-    return ErrnoErrorf("Could not create staging directory '{}'", staging_dir);
-  }
-
-  if (setfilecon(staging_dir.c_str(), "u:object_r:apex_art_staging_data_file:s0") != 0) {
-    return ErrnoErrorf("Could not set label on staging directory '{}'", staging_dir);
-  }
-
-  return staging_dir;
-}
-
 // Gets the `ApexInfo` associated with the currently active ART APEX.
 std::optional<apex::ApexInfo> GetArtApexInfo(const std::vector<apex::ApexInfo>& info_list) {
   auto it = std::find_if(info_list.begin(), info_list.end(), [](const apex::ApexInfo& info) {
@@ -683,17 +662,21 @@ OnDeviceRefresh::OnDeviceRefresh(const OdrConfig& config)
     : OnDeviceRefresh(config,
                       config.GetArtifactDirectory() + "/" + kCacheInfoFile,
                       std::make_unique<ExecUtils>(),
-                      CheckCompilationSpace) {}
-
-OnDeviceRefresh::OnDeviceRefresh(const OdrConfig& config,
-                                 const std::string& cache_info_filename,
-                                 std::unique_ptr<ExecUtils> exec_utils,
-                                 android::base::function_ref<bool()> check_compilation_space)
+                      CheckCompilationSpace,
+                      setfilecon) {}
+
+OnDeviceRefresh::OnDeviceRefresh(
+    const OdrConfig& config,
+    const std::string& cache_info_filename,
+    std::unique_ptr<ExecUtils> exec_utils,
+    android::base::function_ref<bool()> check_compilation_space,
+    android::base::function_ref<int(const char*, const char*)> setfilecon)
     : config_(config),
       cache_info_filename_(cache_info_filename),
       start_time_(time(nullptr)),
       exec_utils_(std::move(exec_utils)),
-      check_compilation_space_(check_compilation_space) {
+      check_compilation_space_(check_compilation_space),
+      setfilecon_(setfilecon) {
   // Updatable APEXes should not have DEX files in the DEX2OATBOOTCLASSPATH. At the time of
   // writing i18n is a non-updatable APEX and so does appear in the DEX2OATBOOTCLASSPATH.
   dex2oat_boot_classpath_jars_ = Split(config_.GetDex2oatBootClasspath(), ":");
@@ -722,6 +705,28 @@ time_t OnDeviceRefresh::GetSubprocessTimeout() const {
   return std::min(GetExecutionTimeRemaining(), kMaxChildProcessSeconds);
 }
 
+Result<std::string> OnDeviceRefresh::CreateStagingDirectory() const {
+  std::string staging_dir = GetArtApexData() + "/staging";
+
+  std::error_code ec;
+  if (std::filesystem::exists(staging_dir, ec)) {
+    if (std::filesystem::remove_all(staging_dir, ec) < 0) {
+      return Errorf(
+          "Could not remove existing staging directory '{}': {}", staging_dir, ec.message());
+    }
+  }
+
+  if (mkdir(staging_dir.c_str(), S_IRWXU) != 0) {
+    return ErrnoErrorf("Could not create staging directory '{}'", staging_dir);
+  }
+
+  if (setfilecon_(staging_dir.c_str(), "u:object_r:apex_art_staging_data_file:s0") != 0) {
+    return ErrnoErrorf("Could not set label on staging directory '{}'", staging_dir);
+  }
+
+  return staging_dir;
+}
+
 std::optional<std::vector<apex::ApexInfo>> OnDeviceRefresh::GetApexInfoList() const {
   std::optional<apex::ApexInfoList> info_list =
       apex::readApexInfoList(config_.GetApexInfoListFile().c_str());
@@ -1726,21 +1731,6 @@ WARN_UNUSED CompilationResult OnDeviceRefresh::RunDex2oat(
       std::make_pair(artifacts.ImagePath(), artifacts.ImageKind()),
       std::make_pair(artifacts.OatPath(), "oat"),
       std::make_pair(artifacts.VdexPath(), "output-vdex")};
-  std::vector<std::unique_ptr<File>> staging_files;
-  for (const auto& [location, kind] : location_kind_pairs) {
-    std::string staging_location = GetStagingLocation(staging_dir, location);
-    std::unique_ptr<File> staging_file(OS::CreateEmptyFile(staging_location.c_str()));
-    if (staging_file == nullptr) {
-      return CompilationResult::Error(
-          OdrMetrics::Status::kIoError,
-          ART_FORMAT("Failed to create {} file '{}': {}", kind, staging_location, strerror(errno)));
-    }
-    // Don't check the state of the staging file. It doesn't need to be flushed because it's removed
-    // after the compilation regardless of success or failure.
-    staging_file->MarkUnchecked();
-    args.Add(StringPrintf("--%s-fd=%d", kind, staging_file->Fd()));
-    staging_files.emplace_back(std::move(staging_file));
-  }
 
   std::string install_location = Dirname(artifacts.OatPath());
   if (!EnsureDirectoryExists(install_location)) {
@@ -1749,6 +1739,27 @@ WARN_UNUSED CompilationResult OnDeviceRefresh::RunDex2oat(
         ART_FORMAT("Error encountered when preparing directory '{}'", install_location));
   }
 
+  std::vector<std::unique_ptr<File>> output_files;
+  for (const auto& [location, kind] : location_kind_pairs) {
+    std::string output_location =
+        staging_dir.empty() ? location : GetStagingLocation(staging_dir, location);
+    std::unique_ptr<File> output_file(OS::CreateEmptyFile(output_location.c_str()));
+    if (output_file == nullptr) {
+      return CompilationResult::Error(
+          OdrMetrics::Status::kIoError,
+          ART_FORMAT("Failed to create {} file '{}': {}", kind, output_location, strerror(errno)));
+    }
+    args.Add(StringPrintf("--%s-fd=%d", kind, output_file->Fd()));
+    output_files.emplace_back(std::move(output_file));
+  }
+
+  // We don't care about file state on failure.
+  auto cleanup = ScopeGuard([&] {
+    for (const std::unique_ptr<File>& file : output_files) {
+      file->MarkUnchecked();
+    }
+  });
+
   args.Concat(std::move(extra_args));
 
   Timer timer;
@@ -1772,12 +1783,29 @@ WARN_UNUSED CompilationResult OnDeviceRefresh::RunDex2oat(
         dex2oat_result);
   }
 
-  if (!MoveOrEraseFiles(staging_files, install_location)) {
-    return CompilationResult::Error(
-        OdrMetrics::Status::kIoError,
-        ART_FORMAT("Failed to commit artifacts to '{}'", install_location));
+  if (staging_dir.empty()) {
+    for (const std::unique_ptr<File>& file : output_files) {
+      if (file->FlushCloseOrErase() != 0) {
+        return CompilationResult::Error(
+            OdrMetrics::Status::kIoError,
+            ART_FORMAT("Failed to flush close file '{}'", file->GetPath()));
+      }
+    }
+  } else {
+    for (const std::unique_ptr<File>& file : output_files) {
+      if (file->Flush() != 0) {
+        return CompilationResult::Error(OdrMetrics::Status::kIoError,
+                                        ART_FORMAT("Failed to flush file '{}'", file->GetPath()));
+      }
+    }
+    if (!MoveOrEraseFiles(output_files, install_location)) {
+      return CompilationResult::Error(
+          OdrMetrics::Status::kIoError,
+          ART_FORMAT("Failed to commit artifacts to '{}'", install_location));
+    }
   }
 
+  cleanup.Disable();
   return CompilationResult::Dex2oatOk(timer.duration().count(), dex2oat_result);
 }
 
@@ -2103,8 +2131,10 @@ WARN_UNUSED ExitCode OnDeviceRefresh::Compile(OdrMetrics& metrics,
     return ExitCode::kCleanupFailed;
   }
 
-  if (!config_.GetStagingDir().empty()) {
-    staging_dir = config_.GetStagingDir();
+  if (config_.GetCompilationOsMode()) {
+    // We don't need to stage files in CompOS. If the compilation fails (partially or entirely),
+    // CompOS will not sign any artifacts, and odsign will discard CompOS outputs entirely.
+    staging_dir = "";
   } else {
     // Create staging area and assign label for generating compilation artifacts.
     Result<std::string> res = CreateStagingDirectory();
@@ -2116,8 +2146,6 @@ WARN_UNUSED ExitCode OnDeviceRefresh::Compile(OdrMetrics& metrics,
     staging_dir = res.value();
   }
 
-  std::string error_msg;
-
   uint32_t dex2oat_invocation_count = 0;
   uint32_t total_dex2oat_invocation_count = compilation_options.CompilationUnitCount();
   ReportNextBootAnimationProgress(dex2oat_invocation_count, total_dex2oat_invocation_count);
diff --git a/odrefresh/odrefresh.h b/odrefresh/odrefresh.h
index 2bc4076d72..a2398b7dbe 100644
--- a/odrefresh/odrefresh.h
+++ b/odrefresh/odrefresh.h
@@ -167,7 +167,8 @@ class OnDeviceRefresh final {
   OnDeviceRefresh(const OdrConfig& config,
                   const std::string& cache_info_filename,
                   std::unique_ptr<ExecUtils> exec_utils,
-                  android::base::function_ref<bool()> check_compilation_space);
+                  android::base::function_ref<bool()> check_compilation_space,
+                  android::base::function_ref<int(const char*, const char*)> setfilecon);
 
   // Returns the exit code and specifies what should be compiled in `compilation_options`.
   WARN_UNUSED ExitCode
@@ -192,6 +193,8 @@ class OnDeviceRefresh final {
 
   time_t GetSubprocessTimeout() const;
 
+  android::base::Result<std::string> CreateStagingDirectory() const;
+
   // Gets the `ApexInfo` for active APEXes.
   std::optional<std::vector<com::android::apex::ApexInfo>> GetApexInfoList() const;
 
@@ -387,6 +390,7 @@ class OnDeviceRefresh final {
   std::unique_ptr<ExecUtils> exec_utils_;
 
   android::base::function_ref<bool()> check_compilation_space_;
+  android::base::function_ref<int(const char*, const char*)> setfilecon_;
 
   DISALLOW_COPY_AND_ASSIGN(OnDeviceRefresh);
 };
diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc
index e482095566..85bcf5c48d 100644
--- a/odrefresh/odrefresh_main.cc
+++ b/odrefresh/odrefresh_main.cc
@@ -155,7 +155,8 @@ int InitializeConfig(int argc, char** argv, OdrConfig* config) {
     } else if (ArgumentMatches(arg, "--system-server-compiler-filter=", &value)) {
       config->SetSystemServerCompilerFilter(value);
     } else if (ArgumentMatches(arg, "--staging-dir=", &value)) {
-      config->SetStagingDir(value);
+      // Keep this for compatibility with CompOS in old platforms.
+      LOG(WARNING) << "--staging-dir is deprecated and its value is ignored";
     } else if (ArgumentEquals(arg, "--dry-run")) {
       config->SetDryRun();
     } else if (ArgumentMatches(arg, "--partial-compilation=", &value)) {
@@ -238,8 +239,6 @@ NO_RETURN void UsageHelp(const char* argv0) {
   UsageMsg("                                 OS.");
   UsageMsg("--dalvik-cache=<DIR>             Write artifacts to .../<DIR> rather than");
   UsageMsg("                                 .../dalvik-cache");
-  UsageMsg("--staging-dir=<DIR>              Write temporary artifacts to <DIR> rather than");
-  UsageMsg("                                 .../staging");
   UsageMsg("--zygote-arch=<STRING>           Zygote kind that overrides ro.zygote");
   UsageMsg("--boot-image-compiler-filter=<STRING>");
   UsageMsg("                                 Compiler filter for the boot image. Default: ");
diff --git a/odrefresh/odrefresh_test.cc b/odrefresh/odrefresh_test.cc
index ae2cb3f6a9..91e1f66803 100644
--- a/odrefresh/odrefresh_test.cc
+++ b/odrefresh/odrefresh_test.cc
@@ -226,19 +226,18 @@ class OdRefreshTest : public CommonArtTest {
     config_.SetSystemServerCompilerFilter("");
     config_.SetArtifactDirectory(dalvik_cache_dir_);
 
-    std::string staging_dir = dalvik_cache_dir_ + "/staging";
-    ASSERT_TRUE(EnsureDirectoryExists(staging_dir));
-    config_.SetStagingDir(staging_dir);
-
     auto mock_exec_utils = std::make_unique<MockExecUtils>();
     mock_exec_utils_ = mock_exec_utils.get();
 
     metrics_ = std::make_unique<OdrMetrics>(dalvik_cache_dir_);
     cache_info_xml_ = dalvik_cache_dir_ + "/cache-info.xml";
+    check_compilation_space_ = [] { return true; };
+    setfilecon_ = [](auto, auto) { return 0; };
     odrefresh_ = std::make_unique<OnDeviceRefresh>(config_,
                                                    cache_info_xml_,
                                                    std::move(mock_exec_utils),
-                                                   /*check_compilation_space=*/[] { return true; });
+                                                   check_compilation_space_,
+                                                   setfilecon_);
   }
 
   void TearDown() override {
@@ -275,6 +274,8 @@ class OdRefreshTest : public CommonArtTest {
   std::string dirty_image_objects_file_;
   std::string preloaded_classes_file_;
   std::string cache_info_xml_;
+  std::function<bool()> check_compilation_space_;
+  std::function<int(const char*, const char*)> setfilecon_;
 };
 
 TEST_F(OdRefreshTest, PrimaryBootImage) {
diff --git a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
index 75f540db1e..59484fe500 100644
--- a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
+++ b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
@@ -325,30 +325,40 @@ public class OdrefreshHostTest extends BaseHostJUnit4Test {
 
     @Test
     public void verifyCompilationOsMode() throws Exception {
-        mTestUtils.removeCompilationLogToAvoidBackoff();
-        mDeviceState.simulateApexUpgrade();
-        long timeMs = mTestUtils.getCurrentTimeMs();
-        mTestUtils.runOdrefresh("--compilation-os-mode");
-
-        mTestUtils.assertNotModifiedAfter(mTestUtils.getExpectedPrimaryBootImage(), timeMs);
-        mTestUtils.assertModifiedAfter(mTestUtils.getExpectedBootImageMainlineExtension(), timeMs);
-        mTestUtils.assertModifiedAfter(mTestUtils.getSystemServerExpectedArtifacts(), timeMs);
-
-        String cacheInfo = getDevice().pullFileContents(OdsignTestUtils.CACHE_INFO_FILE);
-        assertThat(cacheInfo).contains("compilationOsMode=\"true\"");
-
-        // Compilation OS does not write the compilation log to the host.
-        mTestUtils.removeCompilationLogToAvoidBackoff();
-
-        // Simulate the odrefresh invocation on the next boot.
-        timeMs = mTestUtils.getCurrentTimeMs();
-        mTestUtils.runOdrefresh();
-
-        // odrefresh should not re-compile anything.
-        mTestUtils.assertNotModifiedAfter(mTestUtils.getExpectedPrimaryBootImage(), timeMs);
-        mTestUtils.assertNotModifiedAfter(
-                mTestUtils.getExpectedBootImageMainlineExtension(), timeMs);
-        mTestUtils.assertNotModifiedAfter(mTestUtils.getSystemServerExpectedArtifacts(), timeMs);
+        try {
+            // In CompOS, dex2oat directly writes to the output dir. This is allowed on Microdroid
+            // but not allowed on Android, so we need to bypass the SELinux restriction.
+            mTestUtils.assertCommandSucceeds("setenforce 0");
+
+            mTestUtils.removeCompilationLogToAvoidBackoff();
+            mDeviceState.simulateApexUpgrade();
+            long timeMs = mTestUtils.getCurrentTimeMs();
+            mTestUtils.runOdrefresh("--compilation-os-mode");
+
+            mTestUtils.assertNotModifiedAfter(mTestUtils.getExpectedPrimaryBootImage(), timeMs);
+            mTestUtils.assertModifiedAfter(
+                    mTestUtils.getExpectedBootImageMainlineExtension(), timeMs);
+            mTestUtils.assertModifiedAfter(mTestUtils.getSystemServerExpectedArtifacts(), timeMs);
+
+            String cacheInfo = getDevice().pullFileContents(OdsignTestUtils.CACHE_INFO_FILE);
+            assertThat(cacheInfo).contains("compilationOsMode=\"true\"");
+
+            // Compilation OS does not write the compilation log to the host.
+            mTestUtils.removeCompilationLogToAvoidBackoff();
+
+            // Simulate the odrefresh invocation on the next boot.
+            timeMs = mTestUtils.getCurrentTimeMs();
+            mTestUtils.runOdrefresh();
+
+            // odrefresh should not re-compile anything.
+            mTestUtils.assertNotModifiedAfter(mTestUtils.getExpectedPrimaryBootImage(), timeMs);
+            mTestUtils.assertNotModifiedAfter(
+                    mTestUtils.getExpectedBootImageMainlineExtension(), timeMs);
+            mTestUtils.assertNotModifiedAfter(
+                    mTestUtils.getSystemServerExpectedArtifacts(), timeMs);
+        } finally {
+            mTestUtils.assertCommandSucceeds("setenforce 1");
+        }
     }
 
     @Test