Don't activate orphaned APEXes

If an OTA update removes a pre-installed APEX, apexd won't activate it's
/data counterpart.

To achieve this, scanPackagesDirAndActivate is split into two parts:
* ScanApexFiles
* ActivateApexPackages

Note that in order to minimize difference in behaviour, other logic
(e.g. activating immediately after scanning) is preserved. Although it
can be changed in the future to avoid unnecessary mounts.

Test: atest --test-mapping system/apex/apexd
Test: atest apexd_host_tests
Bug: 137086602
Change-Id: Ic546f6869984c2df7e9deaf9d7b87d2195406f23
Merged-In: Ic546f6869984c2df7e9deaf9d7b87d2195406f23
diff --git a/apexd/apex_preinstalled_data.cpp b/apexd/apex_preinstalled_data.cpp
index aeaec64..d62c28c 100644
--- a/apexd/apex_preinstalled_data.cpp
+++ b/apexd/apex_preinstalled_data.cpp
@@ -126,5 +126,9 @@
   return gScannedPreinstalledData[name].path;
 }
 
+bool HasPreInstalledVersion(const std::string& name) {
+  return gScannedPreinstalledData.find(name) != gScannedPreinstalledData.end();
+}
+
 }  // namespace apex
 }  // namespace android
diff --git a/apexd/apex_preinstalled_data.h b/apexd/apex_preinstalled_data.h
index 09b3f7b..1b80aad 100644
--- a/apexd/apex_preinstalled_data.h
+++ b/apexd/apex_preinstalled_data.h
@@ -29,6 +29,6 @@
 android::base::Result<const std::string> getApexKey(const std::string& name);
 android::base::Result<const std::string> getApexPreinstalledPath(
     const std::string& name);
-
+bool HasPreInstalledVersion(const std::string& name);
 }  // namespace apex
 }  // namespace android
diff --git a/apexd/apexd.cpp b/apexd/apexd.cpp
index 6445e6b..c30a941 100644
--- a/apexd/apexd.cpp
+++ b/apexd/apexd.cpp
@@ -63,6 +63,7 @@
 #include <filesystem>
 #include <fstream>
 #include <iomanip>
+#include <iterator>
 #include <memory>
 #include <optional>
 #include <string>
@@ -1222,7 +1223,10 @@
   }
 }
 
-Result<void> scanPackagesDirAndActivate(const char* apex_package_dir) {
+// TODO(ioffe): cleanup activation logic to avoid unnecessary scanning.
+namespace {
+
+Result<std::vector<ApexFile>> ScanApexFiles(const char* apex_package_dir) {
   LOG(INFO) << "Scanning " << apex_package_dir << " looking for APEX packages.";
   if (access(apex_package_dir, F_OK) != 0 && errno == ENOENT) {
     LOG(INFO) << "... does not exist. Skipping";
@@ -1233,52 +1237,61 @@
     return Error() << "Failed to scan " << apex_package_dir << " : "
                    << scan.error();
   }
-
-  const auto& packages_with_code = GetActivePackagesMap();
-
-  std::vector<std::string> failed_pkgs;
-  size_t activated_cnt = 0;
-  size_t skipped_cnt = 0;
-  for (const std::string& name : *scan) {
+  std::vector<ApexFile> ret;
+  for (const auto& name : *scan) {
     LOG(INFO) << "Found " << name;
-
     Result<ApexFile> apex_file = ApexFile::Open(name);
     if (!apex_file.ok()) {
-      LOG(ERROR) << "Failed to activate " << name << " : " << apex_file.error();
-      failed_pkgs.push_back(name);
-      continue;
+      LOG(ERROR) << "Failed to scan " << name << " : " << apex_file.error();
+    } else {
+      ret.emplace_back(std::move(*apex_file));
     }
+  }
+  return ret;
+}
 
-    uint64_t new_version =
-        static_cast<uint64_t>(apex_file->GetManifest().version());
-    const auto& it = packages_with_code.find(apex_file->GetManifest().name());
+Result<void> ActivateApexPackages(const std::vector<ApexFile>& apexes) {
+  const auto& packages_with_code = GetActivePackagesMap();
+  size_t failed_cnt = 0;
+  size_t skipped_cnt = 0;
+  size_t activated_cnt = 0;
+  for (const auto& apex : apexes) {
+    uint64_t new_version = static_cast<uint64_t>(apex.GetManifest().version());
+    const auto& it = packages_with_code.find(apex.GetManifest().name());
     if (it != packages_with_code.end() && it->second >= new_version) {
-      LOG(INFO) << "Skipping activation of " << name
+      LOG(INFO) << "Skipping activation of " << apex.GetPath()
                 << " same package with higher version " << it->second
                 << " is already active";
       skipped_cnt++;
       continue;
     }
 
-    Result<void> res = activatePackageImpl(*apex_file);
-    if (!res.ok()) {
-      LOG(ERROR) << "Failed to activate " << name << " : " << res.error();
-      failed_pkgs.push_back(name);
+    if (auto res = activatePackageImpl(apex); !res.ok()) {
+      LOG(ERROR) << "Failed to activate " << apex.GetPath() << " : "
+                 << res.error();
+      failed_cnt++;
     } else {
       activated_cnt++;
     }
   }
-
-  if (!failed_pkgs.empty()) {
-    return Error() << "Failed to activate following packages : "
-                   << Join(failed_pkgs, ',');
+  if (failed_cnt > 0) {
+    return Error() << "Failed to activate " << failed_cnt << " APEX packages";
   }
-
   LOG(INFO) << "Activated " << activated_cnt
             << " packages. Skipped: " << skipped_cnt;
   return {};
 }
 
+}  // namespace
+
+Result<void> scanPackagesDirAndActivate(const char* apex_package_dir) {
+  auto apexes = ScanApexFiles(apex_package_dir);
+  if (!apexes.ok()) {
+    return apexes.error();
+  }
+  return ActivateApexPackages(*apexes);
+}
+
 /**
  * Snapshots data from base_dir/apexdata/<apex name> to
  * base_dir/apexrollback/<rollback id>/<apex name>.
@@ -1723,11 +1736,16 @@
   }
 
   // Activate built-in APEXes for processes launched before /data is mounted.
-  for (auto const& dir : bootstrap_apex_dirs) {
-    status = scanPackagesDirAndActivate(dir.c_str());
-    if (!status.ok()) {
+  for (const auto& dir : bootstrap_apex_dirs) {
+    auto scan_status = ScanApexFiles(dir.c_str());
+    if (!scan_status.ok()) {
+      LOG(ERROR) << "Failed to scan APEX files in " << dir << " : "
+                 << scan_status.error();
+      return 1;
+    }
+    if (auto ret = ActivateApexPackages(*scan_status); !ret.ok()) {
       LOG(ERROR) << "Failed to activate APEX files in " << dir << " : "
-                 << status.error();
+                 << ret.error();
       return 1;
     }
   }
@@ -1860,25 +1878,52 @@
     LOG(ERROR) << "Failed to resume rollback : " << status.error();
   }
 
-  status = scanPackagesDirAndActivate(kActiveApexPackagesDataDir);
-  if (!status.ok()) {
+  std::vector<ApexFile> data_apex;
+  if (auto scan = ScanApexFiles(kActiveApexPackagesDataDir); !scan.ok()) {
+    LOG(ERROR) << "Failed to scan packages from " << kActiveApexPackagesDataDir
+               << " : " << scan.error();
+    if (auto rollback = rollbackActiveSessionAndReboot(""); !rollback.ok()) {
+      LOG(ERROR) << "Failed to rollback : " << rollback.error();
+    }
+  } else {
+    // Don't activate orphaned APEX packages that don't have a pre-installed
+    // version.
+    auto filter_fn = [](const auto& apex) {
+      if (HasPreInstalledVersion(apex.GetManifest().name())) {
+        return true;
+      }
+      LOG(WARNING) << "Skipping " << apex.GetPath()
+                   << " because there is no pre-installed version";
+      return false;
+    };
+    std::copy_if(std::make_move_iterator(scan->begin()),
+                 std::make_move_iterator(scan->end()),
+                 std::back_inserter(data_apex), filter_fn);
+  }
+
+  if (auto ret = ActivateApexPackages(data_apex); !ret.ok()) {
     LOG(ERROR) << "Failed to activate packages from "
-               << kActiveApexPackagesDataDir << " : " << status.error();
-    Result<void> rollback_status = rollbackActiveSessionAndReboot("");
-    if (!rollback_status.ok()) {
-      // TODO: should we kill apexd in this case?
-      LOG(ERROR) << "Failed to rollback : " << rollback_status.error();
+               << kActiveApexPackagesDataDir << " : " << ret.error();
+    if (auto rollback = rollbackActiveSessionAndReboot(""); !rollback.ok()) {
+      LOG(ERROR) << "Failed to rollback : " << rollback.error();
     }
   }
 
+  // Now also scan and activate APEXes from pre-installed directories.
   for (const auto& dir : kApexPackageBuiltinDirs) {
-    // TODO(b/123622800): if activation failed, rollback and reboot.
-    status = scanPackagesDirAndActivate(dir.c_str());
-    if (!status.ok()) {
+    auto scan = ScanApexFiles(dir.c_str());
+    if (!scan.ok()) {
+      LOG(ERROR) << "Failed to scan APEX packages from " << dir << " : "
+                 << scan.error();
+      if (auto rollback = rollbackActiveSessionAndReboot(""); !rollback.ok()) {
+        LOG(ERROR) << "Failed to rollback : " << rollback.error();
+      }
+    }
+    if (auto activate = ActivateApexPackages(*scan); !activate.ok()) {
       // This should never happen. Like **really** never.
       // TODO: should we kill apexd in this case?
       LOG(ERROR) << "Failed to activate packages from " << dir << " : "
-                 << status.error();
+                 << activate.error();
     }
   }
 
diff --git a/apexd/apexd.h b/apexd/apexd.h
index 0549ce2..c5ea0bb 100644
--- a/apexd/apexd.h
+++ b/apexd/apexd.h
@@ -33,10 +33,11 @@
 
 android::base::Result<void> resumeRollbackIfNeeded();
 
+// Keep it for now to make otapreopt_chroot keep happy.
+// TODO(b/137086602): remove this function.
 android::base::Result<void> scanPackagesDirAndActivate(
     const char* apex_package_dir);
 void scanStagedSessionsDirAndStage();
-
 android::base::Result<void> preinstallPackages(
     const std::vector<std::string>& paths) WARN_UNUSED;
 android::base::Result<void> postinstallPackages(
diff --git a/tests/Android.bp b/tests/Android.bp
index f09a982..f360503 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -239,3 +239,16 @@
     test_suites: ["general-tests"],
     data: [":com.android.apex.cts.shim.v2_prebuilt"],
 }
+
+java_test_host {
+    name: "apexd_host_tests",
+    srcs:  ["src/**/ApexdHostTest.java"],
+    libs: ["tradefed"],
+    static_libs: [
+        "module_test_util",
+        "truth-prebuilt",
+        "apex_manifest_proto_java",
+    ],
+    test_config: "apexd-host-tests.xml",
+    data: [":apex.apexd_test_v2"],
+}
\ No newline at end of file
diff --git a/tests/TEST_MAPPING b/tests/TEST_MAPPING
index 0edbe9b..984288e 100644
--- a/tests/TEST_MAPPING
+++ b/tests/TEST_MAPPING
@@ -10,6 +10,9 @@
       "name": "adbd_e2e_tests"
     },
     {
+      "name": "apexd_host_tests"
+    },
+    {
       "name": "conscrypt_e2e_tests"
     },
     {
diff --git a/tests/apexd-host-tests.xml b/tests/apexd-host-tests.xml
new file mode 100644
index 0000000..06613cc
--- /dev/null
+++ b/tests/apexd-host-tests.xml
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright (C) 2020 The Android Open Source Project
+
+     Licensed under the Apache License, Version 2.0 (the "License");
+     you may not use this file except in compliance with the License.
+     You may obtain a copy of the License at
+
+          http://www.apache.org/licenses/LICENSE-2.0
+
+     Unless required by applicable law or agreed to in writing, software
+     distributed under the License is distributed on an "AS IS" BASIS,
+     WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     See the License for the specific language governing permissions and
+     limitations under the License.
+-->
+<configuration description="Runs the apexd host side test cases">
+    <option name="test-suite-tag" value="apexd_host_tests" />
+    <option name="test-suite-tag" value="apct" />
+    <target_preparer class="com.android.tradefed.targetprep.RootTargetPreparer"/>
+    <test class="com.android.tradefed.testtype.HostTest" >
+        <option name="jar" value="apexd_host_tests.jar" />
+    </test>
+</configuration>
diff --git a/tests/src/com/android/tests/apex/ApexdHostTest.java b/tests/src/com/android/tests/apex/ApexdHostTest.java
new file mode 100644
index 0000000..acd5225
--- /dev/null
+++ b/tests/src/com/android/tests/apex/ApexdHostTest.java
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.tests.apex;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tests.util.ModuleTestUtils;
+import com.android.tradefed.device.ITestDevice;
+import com.android.tradefed.testtype.DeviceJUnit4ClassRunner;
+import com.android.tradefed.testtype.junit4.BaseHostJUnit4Test;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import java.time.Duration;
+import java.util.Set;
+
+/**
+ * Host side integration tests for apexd.
+ */
+@RunWith(DeviceJUnit4ClassRunner.class)
+public class ApexdHostTest extends BaseHostJUnit4Test  {
+
+    private final ModuleTestUtils mTestUtils = new ModuleTestUtils(this);
+
+    @Test
+    public void testOrphanedApexIsNotActivated() throws Exception {
+        assumeTrue("Device requires root", getDevice().isAdbRoot());
+        try {
+            assertThat(getDevice().pushFile(mTestUtils.getTestFile("apex.apexd_test_v2.apex"),
+                    "/data/apex/active/apexd_test_v2.apex")).isTrue();
+            getDevice().reboot();
+            assertWithMessage("Timed out waiting for device to boot").that(
+                    getDevice().waitForBootComplete(Duration.ofMinutes(2).toMillis())).isTrue();
+            final Set<ITestDevice.ApexInfo> activeApexes = getDevice().getActiveApexes();
+            ITestDevice.ApexInfo testApex = new ITestDevice.ApexInfo(
+                    "com.android.apex.test_package",
+                    2L);
+            assertThat(activeApexes).doesNotContain(testApex);
+            // TODO(b/137086602): check that /data/apex/active/apexd_test_v2.apex was deleted.
+        } finally {
+            getDevice().executeShellV2Command("rm /data/apex/active/apexd_test_v2.apex");
+        }
+    }
+}