Merge changes If6611d64,Ie55c3ac1

* changes:
  init: remove unneeded special cases from FscryptInferAction
  init: fix mkdir to reliably detect top-level /data directories

init/util.cpp

@@ -61,6 +61,8 @@ namespace init {
 
 const std::string kDefaultAndroidDtDir("/proc/device-tree/firmware/android/");
 
+const std::string kDataDirPrefix("/data/");
+
 void (*trigger_shutdown)(const std::string& command) = nullptr;
 
 // DecodeUid() - decodes and returns the given string, which can be either the
@@ -458,52 +460,34 @@ Result<void> IsLegalPropertyValue(const std::string& name, const std::string& va
     return {};
 }
 
-static FscryptAction FscryptInferAction(const std::string& dir) {
+// Remove unnecessary slashes so that any later checks (e.g., the check for
-    const std::string prefix = "/data/";
+// whether the path is a top-level directory in /data) don't get confused.
-
+std::string CleanDirPath(const std::string& path) {
-    if (!android::base::StartsWith(dir, prefix)) {
+    std::string result;
-        return FscryptAction::kNone;
+    result.reserve(path.length());
-    }
+    // Collapse duplicate slashes, e.g. //data//foo// => /data/foo/
-
+    for (char c : path) {
-    // Only set policy on first level /data directories
+        if (c != '/' || result.empty() || result.back() != '/') {
-    // To make this less restrictive, consider using a policy file.
+            result += c;
-    // However this is overkill for as long as the policy is simply
-    // to apply a global policy to all /data folders created via makedir
-    if (dir.find_first_of('/', prefix.size()) != std::string::npos) {
-        return FscryptAction::kNone;
-    }
-
-    // Special case various directories that must not be encrypted,
-    // often because their subdirectories must be encrypted.
-    // This isn't a nice way to do this, see b/26641735
-    std::vector<std::string> directories_to_exclude = {
-            "lost+found", "system_ce", "system_de", "misc_ce",     "misc_de",
-            "vendor_ce",  "vendor_de", "media",     "data",        "user",
-            "user_de",    "apex",      "preloads",  "app-staging", "gsi",
-    };
-    for (const auto& d : directories_to_exclude) {
-        if ((prefix + d) == dir) {
-            return FscryptAction::kNone;
         }
     }
-    // Empty these directories if policy setting fails.
+    // Remove trailing slash, e.g. /data/foo/ => /data/foo
-    std::vector<std::string> wipe_on_failure = {
+    if (result.length() > 1 && result.back() == '/') {
-            "rollback", "rollback-observer",  // b/139193659
+        result.pop_back();
-    };
-    for (const auto& d : wipe_on_failure) {
-        if ((prefix + d) == dir) {
-            return FscryptAction::kDeleteIfNecessary;
-        }
     }
-    return FscryptAction::kRequire;
+    return result;
 }
 
 Result<MkdirOptions> ParseMkdir(const std::vector<std::string>& args) {
+    std::string path = CleanDirPath(args[1]);
+    const bool is_toplevel_data_dir =
+            StartsWith(path, kDataDirPrefix) &&
+            path.find_first_of('/', kDataDirPrefix.size()) == std::string::npos;
+    FscryptAction fscrypt_action =
+            is_toplevel_data_dir ? FscryptAction::kRequire : FscryptAction::kNone;
     mode_t mode = 0755;
     Result<uid_t> uid = -1;
     Result<gid_t> gid = -1;
-    FscryptAction fscrypt_inferred_action = FscryptInferAction(args[1]);
-    FscryptAction fscrypt_action = fscrypt_inferred_action;
     std::string ref_option = "ref";
     bool set_option_encryption = false;
     bool set_option_key = false;
@@ -568,24 +552,17 @@ Result<MkdirOptions> ParseMkdir(const std::vector<std::string>& args) {
     if (set_option_key && fscrypt_action == FscryptAction::kNone) {
         return Error() << "Key option set but encryption action is none";
     }
-    const std::string prefix = "/data/";
+    if (is_toplevel_data_dir) {
-    if (StartsWith(args[1], prefix) &&
-        args[1].find_first_of('/', prefix.size()) == std::string::npos) {
         if (!set_option_encryption) {
-            LOG(WARNING) << "Top-level directory needs encryption action, eg mkdir " << args[1]
+            LOG(WARNING) << "Top-level directory needs encryption action, eg mkdir " << path
                          << " <mode> <uid> <gid> encryption=Require";
         }
         if (fscrypt_action == FscryptAction::kNone) {
-            LOG(INFO) << "Not setting encryption policy on: " << args[1];
+            LOG(INFO) << "Not setting encryption policy on: " << path;
         }
     }
-    if (fscrypt_action != fscrypt_inferred_action) {
-        LOG(WARNING) << "Inferred action different from explicit one, expected "
-                     << static_cast<int>(fscrypt_inferred_action) << " but got "
-                     << static_cast<int>(fscrypt_action);
-    }
 
-    return MkdirOptions{args[1], mode, *uid, *gid, fscrypt_action, ref_option};
+    return MkdirOptions{path, mode, *uid, *gid, fscrypt_action, ref_option};
 }
 
 Result<MountAllOptions> ParseMountAll(const std::vector<std::string>& args) {

init/util.h

@@ -69,6 +69,7 @@ bool is_android_dt_value_expected(const std::string& sub_path, const std::string
 
 bool IsLegalPropertyName(const std::string& name);
 Result<void> IsLegalPropertyValue(const std::string& name, const std::string& value);
+std::string CleanDirPath(const std::string& path);
 
 struct MkdirOptions {
     std::string target;

init/util_test.cpp

@@ -170,5 +170,18 @@ TEST(util, mkdir_recursive_extra_slashes) {
     EXPECT_TRUE(is_dir(path1.c_str()));
 }
 
+TEST(util, CleanDirPath) {
+    EXPECT_EQ("", CleanDirPath(""));
+    EXPECT_EQ("/", CleanDirPath("/"));
+    EXPECT_EQ("/", CleanDirPath("//"));
+    EXPECT_EQ("/foo", CleanDirPath("/foo"));
+    EXPECT_EQ("/foo", CleanDirPath("//foo"));
+    EXPECT_EQ("/foo", CleanDirPath("/foo/"));
+    EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar"));
+    EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar/"));
+    EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar////"));
+    EXPECT_EQ("/foo/bar", CleanDirPath("//foo//bar"));
+}
+
 }  // namespace init
 }  // namespace android