Merge "Prefer f2fs for virtual (private) storage volumes." into rvc-dev
diff --git a/Android.bp b/Android.bp
index c2f8936..03dde1e 100644
--- a/Android.bp
+++ b/Android.bp
@@ -131,6 +131,7 @@
         "ScryptParameters.cpp",
         "Utils.cpp",
         "VoldNativeService.cpp",
+        "VoldNativeServiceValidation.cpp",
         "VoldUtil.cpp",
         "VolumeManager.cpp",
         "cryptfs.cpp",
diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp
index 08b4661..1beb29c 100644
--- a/VoldNativeService.cpp
+++ b/VoldNativeService.cpp
@@ -18,23 +18,6 @@
 
 #include "VoldNativeService.h"
 
-#include "Benchmark.h"
-#include "CheckEncryption.h"
-#include "Checkpoint.h"
-#include "FsCrypt.h"
-#include "IdleMaint.h"
-#include "MetadataCrypt.h"
-#include "MoveStorage.h"
-#include "Process.h"
-#include "VoldUtil.h"
-#include "VolumeManager.h"
-#include "cryptfs.h"
-
-#include "incfs_ndk.h"
-
-#include <fstream>
-#include <thread>
-
 #include <android-base/logging.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
@@ -43,8 +26,26 @@
 #include <private/android_filesystem_config.h>
 #include <utils/Trace.h>
 
+#include <fstream>
+#include <thread>
+
+#include "Benchmark.h"
+#include "CheckEncryption.h"
+#include "Checkpoint.h"
+#include "FsCrypt.h"
+#include "IdleMaint.h"
+#include "MetadataCrypt.h"
+#include "MoveStorage.h"
+#include "Process.h"
+#include "VoldNativeServiceValidation.h"
+#include "VoldUtil.h"
+#include "VolumeManager.h"
+#include "cryptfs.h"
+#include "incfs_ndk.h"
+
 using android::base::StringPrintf;
 using std::endl;
+using namespace std::literals;
 
 namespace android {
 namespace vold {
@@ -53,14 +54,6 @@
 
 constexpr const char* kDump = "android.permission.DUMP";
 
-static binder::Status ok() {
-    return binder::Status::ok();
-}
-
-static binder::Status exception(uint32_t code, const std::string& msg) {
-    return binder::Status::fromExceptionCode(code, String8(msg.c_str()));
-}
-
 static binder::Status error(const std::string& msg) {
     PLOG(ERROR) << msg;
     return binder::Status::fromServiceSpecificError(errno, String8(msg.c_str()));
@@ -82,77 +75,9 @@
     }
 }
 
-binder::Status checkPermission(const char* permission) {
-    pid_t pid;
-    uid_t uid;
-
-    if (checkCallingPermission(String16(permission), reinterpret_cast<int32_t*>(&pid),
-                               reinterpret_cast<int32_t*>(&uid))) {
-        return ok();
-    } else {
-        return exception(binder::Status::EX_SECURITY,
-                         StringPrintf("UID %d / PID %d lacks permission %s", uid, pid, permission));
-    }
-}
-
-binder::Status checkUidOrRoot(uid_t expectedUid) {
-    uid_t uid = IPCThreadState::self()->getCallingUid();
-    if (uid == expectedUid || uid == AID_ROOT) {
-        return ok();
-    } else {
-        return exception(binder::Status::EX_SECURITY,
-                         StringPrintf("UID %d is not expected UID %d", uid, expectedUid));
-    }
-}
-
-binder::Status checkArgumentId(const std::string& id) {
-    if (id.empty()) {
-        return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing ID");
-    }
-    for (const char& c : id) {
-        if (!std::isalnum(c) && c != ':' && c != ',' && c != ';') {
-            return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
-                             StringPrintf("ID %s is malformed", id.c_str()));
-        }
-    }
-    return ok();
-}
-
-binder::Status checkArgumentPath(const std::string& path) {
-    if (path.empty()) {
-        return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing path");
-    }
-    if (path[0] != '/') {
-        return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
-                         StringPrintf("Path %s is relative", path.c_str()));
-    }
-    if ((path + '/').find("/../") != std::string::npos) {
-        return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
-                         StringPrintf("Path %s is shady", path.c_str()));
-    }
-    for (const char& c : path) {
-        if (c == '\0' || c == '\n') {
-            return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
-                             StringPrintf("Path %s is malformed", path.c_str()));
-        }
-    }
-    return ok();
-}
-
-binder::Status checkArgumentHex(const std::string& hex) {
-    // Empty hex strings are allowed
-    for (const char& c : hex) {
-        if (!std::isxdigit(c) && c != ':' && c != '-') {
-            return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
-                             StringPrintf("Hex %s is malformed", hex.c_str()));
-        }
-    }
-    return ok();
-}
-
 #define ENFORCE_SYSTEM_OR_ROOT                              \
     {                                                       \
-        binder::Status status = checkUidOrRoot(AID_SYSTEM); \
+        binder::Status status = CheckUidOrRoot(AID_SYSTEM); \
         if (!status.isOk()) {                               \
             return status;                                  \
         }                                                   \
@@ -160,7 +85,7 @@
 
 #define CHECK_ARGUMENT_ID(id)                          \
     {                                                  \
-        binder::Status status = checkArgumentId((id)); \
+        binder::Status status = CheckArgumentId((id)); \
         if (!status.isOk()) {                          \
             return status;                             \
         }                                              \
@@ -168,7 +93,7 @@
 
 #define CHECK_ARGUMENT_PATH(path)                          \
     {                                                      \
-        binder::Status status = checkArgumentPath((path)); \
+        binder::Status status = CheckArgumentPath((path)); \
         if (!status.isOk()) {                              \
             return status;                                 \
         }                                                  \
@@ -176,7 +101,7 @@
 
 #define CHECK_ARGUMENT_HEX(hex)                          \
     {                                                    \
-        binder::Status status = checkArgumentHex((hex)); \
+        binder::Status status = CheckArgumentHex((hex)); \
         if (!status.isOk()) {                            \
             return status;                               \
         }                                                \
@@ -206,7 +131,7 @@
 
 status_t VoldNativeService::dump(int fd, const Vector<String16>& /* args */) {
     auto out = std::fstream(StringPrintf("/proc/self/fd/%d", fd));
-    const binder::Status dump_permission = checkPermission(kDump);
+    const binder::Status dump_permission = CheckPermission(kDump);
     if (!dump_permission.isOk()) {
         out << dump_permission.toString8() << endl;
         return PERMISSION_DENIED;
@@ -214,17 +139,16 @@
 
     ACQUIRE_LOCK;
     out << "vold is happy!" << endl;
-    out.flush();
     return NO_ERROR;
 }
 
 binder::Status VoldNativeService::setListener(
-    const android::sp<android::os::IVoldListener>& listener) {
+        const android::sp<android::os::IVoldListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     ACQUIRE_LOCK;
 
     VolumeManager::Instance()->setListener(listener);
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::monitor() {
@@ -234,7 +158,7 @@
     { ACQUIRE_LOCK; }
     { ACQUIRE_CRYPT_LOCK; }
 
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::reset() {
@@ -281,12 +205,12 @@
 
 binder::Status VoldNativeService::addAppIds(const std::vector<std::string>& packageNames,
                                             const std::vector<int32_t>& appIds) {
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::addSandboxIds(const std::vector<int32_t>& appIds,
                                                 const std::vector<std::string>& sandboxIds) {
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::onSecureKeyguardStateChanged(bool isShowing) {
@@ -403,11 +327,11 @@
             return error("Volume " + volId + " missing path");
         }
     }
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::benchmark(
-    const std::string& volId, const android::sp<android::os::IVoldTaskListener>& listener) {
+        const std::string& volId, const android::sp<android::os::IVoldTaskListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     CHECK_ARGUMENT_ID(volId);
     ACQUIRE_LOCK;
@@ -417,7 +341,7 @@
     if (!status.isOk()) return status;
 
     std::thread([=]() { android::vold::Benchmark(path, listener); }).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::checkEncryption(const std::string& volId) {
@@ -432,8 +356,8 @@
 }
 
 binder::Status VoldNativeService::moveStorage(
-    const std::string& fromVolId, const std::string& toVolId,
-    const android::sp<android::os::IVoldTaskListener>& listener) {
+        const std::string& fromVolId, const std::string& toVolId,
+        const android::sp<android::os::IVoldTaskListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     CHECK_ARGUMENT_ID(fromVolId);
     CHECK_ARGUMENT_ID(toVolId);
@@ -448,7 +372,7 @@
     }
 
     std::thread([=]() { android::vold::MoveStorage(fromVol, toVol, listener); }).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::remountUid(int32_t uid, int32_t remountMode) {
@@ -458,6 +382,14 @@
     return translate(VolumeManager::Instance()->remountUid(uid, remountMode));
 }
 
+binder::Status VoldNativeService::remountAppStorageDirs(int uid, int pid,
+        const std::vector<std::string>& packageNames) {
+    ENFORCE_SYSTEM_OR_ROOT;
+    ACQUIRE_LOCK;
+
+    return translate(VolumeManager::Instance()->remountAppStorageDirs(uid, pid, packageNames));
+}
+
 binder::Status VoldNativeService::setupAppDir(const std::string& path, int32_t appUid) {
     ENFORCE_SYSTEM_OR_ROOT;
     CHECK_ARGUMENT_PATH(path);
@@ -483,7 +415,7 @@
     ACQUIRE_LOCK;
 
     return translate(
-        VolumeManager::Instance()->createObb(sourcePath, sourceKey, ownerGid, _aidl_return));
+            VolumeManager::Instance()->createObb(sourcePath, sourceKey, ownerGid, _aidl_return));
 }
 
 binder::Status VoldNativeService::destroyObb(const std::string& volId) {
@@ -521,30 +453,30 @@
 }
 
 binder::Status VoldNativeService::fstrim(
-    int32_t fstrimFlags, const android::sp<android::os::IVoldTaskListener>& listener) {
+        int32_t fstrimFlags, const android::sp<android::os::IVoldTaskListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     ACQUIRE_LOCK;
 
     std::thread([=]() { android::vold::Trim(listener); }).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::runIdleMaint(
-    const android::sp<android::os::IVoldTaskListener>& listener) {
+        const android::sp<android::os::IVoldTaskListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     ACQUIRE_LOCK;
 
     std::thread([=]() { android::vold::RunIdleMaint(listener); }).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::abortIdleMaint(
-    const android::sp<android::os::IVoldTaskListener>& listener) {
+        const android::sp<android::os::IVoldTaskListener>& listener) {
     ENFORCE_SYSTEM_OR_ROOT;
     ACQUIRE_LOCK;
 
     std::thread([=]() { android::vold::AbortIdleMaint(listener); }).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::mountAppFuse(int32_t uid, int32_t mountId,
@@ -576,7 +508,7 @@
     }
 
     *_aidl_return = android::base::unique_fd(fd);
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fdeCheckPassword(const std::string& password) {
@@ -593,7 +525,7 @@
     // Spawn as thread so init can issue commands back to vold without
     // causing deadlock, usually as a result of prep_data_fs.
     std::thread(&cryptfs_restart).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fdeComplete(int32_t* _aidl_return) {
@@ -601,7 +533,7 @@
     ACQUIRE_CRYPT_LOCK;
 
     *_aidl_return = cryptfs_crypto_complete();
-    return ok();
+    return Ok();
 }
 
 static int fdeEnableInternal(int32_t passwordType, const std::string& password,
@@ -641,7 +573,7 @@
     // Spawn as thread so init can issue commands back to vold without
     // causing deadlock, usually as a result of prep_data_fs.
     std::thread(&fdeEnableInternal, passwordType, password, encryptionFlags).detach();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fdeChangePassword(int32_t passwordType,
@@ -668,7 +600,7 @@
         return error(StringPrintf("Failed to read field %s", key.c_str()));
     } else {
         *_aidl_return = buf;
-        return ok();
+        return Ok();
     }
 }
 
@@ -684,7 +616,7 @@
     ACQUIRE_CRYPT_LOCK;
 
     *_aidl_return = cryptfs_get_password_type();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fdeGetPassword(std::string* _aidl_return) {
@@ -695,7 +627,7 @@
     if (res != nullptr) {
         *_aidl_return = res;
     }
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fdeClearPassword() {
@@ -703,7 +635,7 @@
     ACQUIRE_CRYPT_LOCK;
 
     cryptfs_clear_password();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::fbeEnable() {
@@ -722,7 +654,7 @@
         // causing deadlock, usually as a result of prep_data_fs.
         std::thread(&cryptfs_mount_default_encrypted).detach();
     }
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::initUser0() {
@@ -737,7 +669,7 @@
     ACQUIRE_CRYPT_LOCK;
 
     *_aidl_return = cryptfs_isConvertibleToFBE() != 0;
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::mountFstab(const std::string& blkDevice,
@@ -756,7 +688,8 @@
     return translateBool(fscrypt_mount_metadata_encrypted(blkDevice, mountPoint, true));
 }
 
-binder::Status VoldNativeService::createUserKey(int32_t userId, int32_t userSerial, bool ephemeral) {
+binder::Status VoldNativeService::createUserKey(int32_t userId, int32_t userSerial,
+                                                bool ephemeral) {
     ENFORCE_SYSTEM_OR_ROOT;
     ACQUIRE_CRYPT_LOCK;
 
@@ -837,13 +770,13 @@
 binder::Status VoldNativeService::prepareSandboxForApp(const std::string& packageName,
                                                        int32_t appId, const std::string& sandboxId,
                                                        int32_t userId) {
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::destroySandboxForApp(const std::string& packageName,
                                                        const std::string& sandboxId,
                                                        int32_t userId) {
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::startCheckpoint(int32_t retry) {
@@ -858,7 +791,7 @@
     ACQUIRE_LOCK;
 
     *_aidl_return = cp_needsRollback();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::needsCheckpoint(bool* _aidl_return) {
@@ -866,7 +799,7 @@
     ACQUIRE_LOCK;
 
     *_aidl_return = cp_needsCheckpoint();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::commitChanges() {
@@ -911,7 +844,7 @@
     ACQUIRE_LOCK;
 
     cp_abortChanges(message, retry);
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::supportsCheckpoint(bool* _aidl_return) {
@@ -940,39 +873,54 @@
     ACQUIRE_LOCK;
 
     cp_resetCheckpoint();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::incFsEnabled(bool* _aidl_return) {
+    ENFORCE_SYSTEM_OR_ROOT;
+
     *_aidl_return = IncFs_IsEnabled();
-    return ok();
+    return Ok();
 }
 
 binder::Status VoldNativeService::mountIncFs(
         const std::string& backingPath, const std::string& targetDir, int32_t flags,
         ::android::os::incremental::IncrementalFileSystemControlParcel* _aidl_return) {
-    auto result = IncFs_Mount(backingPath.c_str(), targetDir.c_str(),
-                              {.flags = IncFsMountFlags(flags),
-                               .defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS,
-                               .readLogBufferPages = 4});
-    if (result.cmd < 0) {
-        return translate(result.cmd);
+    ENFORCE_SYSTEM_OR_ROOT;
+    CHECK_ARGUMENT_PATH(backingPath);
+    CHECK_ARGUMENT_PATH(targetDir);
+
+    auto control = IncFs_Mount(backingPath.c_str(), targetDir.c_str(),
+                               {.flags = IncFsMountFlags(flags),
+                                .defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS,
+                                .readLogBufferPages = 4});
+    if (control == nullptr) {
+        return translate(-1);
     }
     using unique_fd = ::android::base::unique_fd;
-    _aidl_return->cmd.reset(unique_fd(result.cmd));
-    _aidl_return->pendingReads.reset(unique_fd(result.pendingReads));
-    if (result.logs >= 0) {
-        _aidl_return->log.reset(unique_fd(result.logs));
+    _aidl_return->cmd.reset(unique_fd(dup(IncFs_GetControlFd(control, CMD))));
+    _aidl_return->pendingReads.reset(unique_fd(dup(IncFs_GetControlFd(control, PENDING_READS))));
+    auto logsFd = IncFs_GetControlFd(control, LOGS);
+    if (logsFd >= 0) {
+        _aidl_return->log.reset(unique_fd(dup(logsFd)));
     }
-    return ok();
+    IncFs_DeleteControl(control);
+    return Ok();
 }
 
 binder::Status VoldNativeService::unmountIncFs(const std::string& dir) {
+    ENFORCE_SYSTEM_OR_ROOT;
+    CHECK_ARGUMENT_PATH(dir);
+
     return translate(IncFs_Unmount(dir.c_str()));
 }
 
 binder::Status VoldNativeService::bindMount(const std::string& sourceDir,
                                             const std::string& targetDir) {
+    ENFORCE_SYSTEM_OR_ROOT;
+    CHECK_ARGUMENT_PATH(sourceDir);
+    CHECK_ARGUMENT_PATH(targetDir);
+
     return translate(IncFs_BindMount(sourceDir.c_str(), targetDir.c_str()));
 }
 
diff --git a/VoldNativeService.h b/VoldNativeService.h
index e04c259..2f4b6eb 100644
--- a/VoldNativeService.h
+++ b/VoldNativeService.h
@@ -64,6 +64,8 @@
                                const android::sp<android::os::IVoldTaskListener>& listener);
 
     binder::Status remountUid(int32_t uid, int32_t remountMode);
+    binder::Status remountAppStorageDirs(int uid, int pid,
+                               const std::vector<std::string>& packageNames);
 
     binder::Status setupAppDir(const std::string& path, int32_t appUid);
     binder::Status fixupAppDir(const std::string& path, int32_t appUid);
diff --git a/VoldNativeServiceValidation.cpp b/VoldNativeServiceValidation.cpp
new file mode 100644
index 0000000..2e21ace
--- /dev/null
+++ b/VoldNativeServiceValidation.cpp
@@ -0,0 +1,109 @@
+/*
+ * 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.
+ */
+
+#include "VoldNativeServiceValidation.h"
+
+#include <android-base/stringprintf.h>
+#include <android-base/strings.h>
+#include <binder/IPCThreadState.h>
+#include <binder/IServiceManager.h>
+#include <private/android_filesystem_config.h>
+
+#include <cctype>
+#include <string_view>
+
+using android::base::StringPrintf;
+using namespace std::literals;
+
+namespace android::vold {
+
+binder::Status Ok() {
+    return binder::Status::ok();
+}
+
+binder::Status Exception(uint32_t code, const std::string& msg) {
+    return binder::Status::fromExceptionCode(code, String8(msg.c_str()));
+}
+
+binder::Status CheckPermission(const char* permission) {
+    pid_t pid;
+    uid_t uid;
+
+    if (checkCallingPermission(String16(permission), reinterpret_cast<int32_t*>(&pid),
+                               reinterpret_cast<int32_t*>(&uid))) {
+        return Ok();
+    } else {
+        return Exception(binder::Status::EX_SECURITY,
+                         StringPrintf("UID %d / PID %d lacks permission %s", uid, pid, permission));
+    }
+}
+
+binder::Status CheckUidOrRoot(uid_t expectedUid) {
+    uid_t uid = IPCThreadState::self()->getCallingUid();
+    if (uid == expectedUid || uid == AID_ROOT) {
+        return Ok();
+    } else {
+        return Exception(binder::Status::EX_SECURITY,
+                         StringPrintf("UID %d is not expected UID %d", uid, expectedUid));
+    }
+}
+
+binder::Status CheckArgumentId(const std::string& id) {
+    if (id.empty()) {
+        return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing ID");
+    }
+    for (const char& c : id) {
+        if (!std::isalnum(c) && c != ':' && c != ',' && c != ';') {
+            return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                             StringPrintf("ID %s is malformed", id.c_str()));
+        }
+    }
+    return Ok();
+}
+
+binder::Status CheckArgumentPath(const std::string& path) {
+    if (path.empty()) {
+        return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing path");
+    }
+    if (path[0] != '/') {
+        return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                         StringPrintf("Path %s is relative", path.c_str()));
+    }
+    if (path.find("/../"sv) != path.npos || android::base::EndsWith(path, "/.."sv)) {
+        return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                         StringPrintf("Path %s is shady", path.c_str()));
+    }
+    for (const char& c : path) {
+        if (c == '\0' || c == '\n') {
+            return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                             StringPrintf("Path %s is malformed", path.c_str()));
+        }
+    }
+    return Ok();
+}
+
+binder::Status CheckArgumentHex(const std::string& hex) {
+    // Empty hex strings are allowed
+    for (const char& c : hex) {
+        if (!std::isxdigit(c) && c != ':' && c != '-') {
+            return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                             StringPrintf("Hex %s is malformed", hex.c_str()));
+        }
+    }
+    return Ok();
+}
+
+}  // namespace android::vold
diff --git a/VoldNativeServiceValidation.h b/VoldNativeServiceValidation.h
new file mode 100644
index 0000000..d2fc9e0
--- /dev/null
+++ b/VoldNativeServiceValidation.h
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <binder/Status.h>
+
+#include <string>
+
+#include <stdint.h>
+#include <sys/types.h>
+
+namespace android::vold {
+
+binder::Status Ok();
+binder::Status Exception(uint32_t code, const std::string& msg);
+
+binder::Status CheckPermission(const char* permission);
+binder::Status CheckUidOrRoot(uid_t expectedUid);
+binder::Status CheckArgumentId(const std::string& id);
+binder::Status CheckArgumentPath(const std::string& path);
+binder::Status CheckArgumentHex(const std::string& hex);
+
+}  // namespace android::vold
diff --git a/VolumeManager.cpp b/VolumeManager.cpp
index fce977e..6a40a52 100644
--- a/VolumeManager.cpp
+++ b/VolumeManager.cpp
@@ -738,228 +738,144 @@
             forkAndRemountChild, &mountMode) ? 0 : -1;
 }
 
-// Bind mount obb & data dir for an app if necessary.
-// How it works:
-// 1). Check if a pid is an app uid and not the FuseDaemon, if not then return.
-// 2). Get the mounts for that pid.
-// 3). If obb is already mounted then return, otherwise we need to mount obb for this pid.
-// 4). Get all packages and uid mounted for jit profile. These packages are all packages with
-// same uid or whitelisted apps.
-// 5a). If there's no package, it means it's not a process running app data isolation, so
-// just bind mount Android/obb & Android/data dir.
-// 5b). Otherwise, for each package, create obb dir if it's not created and bind mount it.
-// TODO: Should we get some reliable data from system server instead of scanning /proc ?
-static bool bindMountAppDataObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, void* params) {
-    if (uid < AID_APP_START || uid > AID_APP_END) {
-        return true;
-    }
-    if (android::vold::IsFuseDaemon(pid)) {
-        return true;
-    }
-    async_safe_format_log(ANDROID_LOG_ERROR, "vold",
-                          "Start mounting obb and data for uid:%d, pid:%d", uid, pid);
 
-    userid_t userId = multiuser_get_user_id(uid);
+// Set the namespace the app process and remount its storage directories.
+static bool remountStorageDirs(int nsFd, const char* sources[], const char* targets[], int size) {
+    // This code is executed after a fork so it's very important that the set of
+    // methods we call here is strictly limited.
     if (setns(nsFd, CLONE_NEWNS) != 0) {
         async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to setns %s", strerror(errno));
         return false;
     }
 
-    std::string profiles_path(StringPrintf("/data/misc/profiles/cur/%d/", userId));
-    // We search both .../obb and .../obb/$PKG paths here.
-    std::string obb_path(StringPrintf("/storage/emulated/%d/Android/obb", userId));
-    int profiles_path_len = profiles_path.length();
-    int obb_path_len = obb_path.length();
-
-    // TODO: Refactor the code as a util function so we can reuse the mount parsing code.
-    std::string mounts_file(StringPrintf("/proc/%d/mounts", pid));
-    auto fp = std::unique_ptr<FILE, int (*)(FILE*)>(
-            setmntent(mounts_file.c_str(), "r"), endmntent);
-    if (!fp) {
-        async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Error opening %s: %s",
-                              mounts_file.c_str(), strerror(errno));
-        return false;
-    }
-
-    // Check if obb directory is mounted, and get all packages of mounted app data directory.
-    // We only need to check obb directory and assume if obb is mounted, data is mounted also.
-    bool obb_mounted = false;
-    std::vector<std::string> pkg_name_list;
-    mntent* mentry;
-    while ((mentry = getmntent(fp.get())) != nullptr) {
-        if (strncmp(mentry->mnt_dir, profiles_path.c_str(), profiles_path_len) == 0) {
-            pkg_name_list.push_back(std::string(mentry->mnt_dir + profiles_path_len));
-        }
-        if (strncmp(mentry->mnt_dir, obb_path.c_str(), obb_path_len) == 0) {
-            obb_mounted = true;
-        }
-    }
-
-    // Obb mounted in zygote already, so skip it
-    if (obb_mounted) {
-        return true;
-    }
-
-    std::string obbSource, dataSource;
-    if (IsFilesystemSupported("sdcardfs")) {
-        obbSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/obb", userId);
-        dataSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/data", userId);
-    } else {
-        obbSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/obb", userId, userId);
-        dataSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/data", userId, userId);
-    }
-    std::string obbTarget(StringPrintf("/storage/emulated/%d/Android/obb", userId));
-    std::string dataTarget(StringPrintf("/storage/emulated/%d/Android/data", userId));
-
-    // TODO: Review if these checks are still necessary
-    auto status = EnsureDirExists(obbSource, 0771, AID_MEDIA_RW, AID_MEDIA_RW);
-    if (status != OK) {
-        async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to create dir %s %s",
-                              obbSource.c_str(), strerror(-status));
-        return false;
-    }
-    status = EnsureDirExists(dataSource, 0771, AID_MEDIA_RW, AID_MEDIA_RW);
-    if (status != OK) {
-        async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to create dir %s %s",
-                              dataSource.c_str(), strerror(-status));
-        return false;
-    }
-
-    // It means app data isolation is not applied to this, so we can just bind the whole obb
-    // directory instead.
-    if (pkg_name_list.empty()) {
-        async_safe_format_log(ANDROID_LOG_INFO, "vold",
-                              "Bind mounting whole obb and data directory for pid %d", pid);
-        auto status1 = BindMount(obbSource, obbTarget);
-        // Still bind mount data even obb fails, just slower to access obb dir
-        auto status2 = BindMount(dataSource, dataTarget);
-        if (status1 != OK) {
-            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s",
-                                  obbSource.c_str(), obbTarget.c_str(), strerror(-status));
+    for (int i = 0; i < size; i++) {
+        if (TEMP_FAILURE_RETRY(mount(sources[i], targets[i], NULL, MS_BIND | MS_REC, NULL)) == -1) {
+            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s to %s :%s",
+                                  sources[i], targets[i], strerror(errno));
             return false;
         }
-        if (status2 != OK) {
-            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s",
-                                  dataSource.c_str(), dataTarget.c_str(), strerror(-status));
-            return false;
-        }
-        return true;
-    }
-
-    // Bind mount each app's obb directory
-    for (const auto& pkg_name : pkg_name_list) {
-        std::string appObbSource, appDataSource;
-        if (IsFilesystemSupported("sdcardfs")) {
-            appObbSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/obb/%s",
-                    userId, pkg_name.c_str());
-            appDataSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/data/%s",
-                    userId, pkg_name.c_str());
-        } else {
-            appObbSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/obb/%s",
-                    userId, userId, pkg_name.c_str());
-            appDataSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/data/%s",
-                    userId, userId, pkg_name.c_str());
-        }
-        std::string appObbTarget(StringPrintf("/storage/emulated/%d/Android/obb/%s",
-                userId, pkg_name.c_str()));
-        std::string appDataTarget(StringPrintf("/storage/emulated/%d/Android/data/%s",
-                userId, pkg_name.c_str()));
-
-        status = EnsureDirExists(appObbSource, 0770, uid, AID_MEDIA_RW);
-        if (status != OK) {
-            async_safe_format_log(ANDROID_LOG_INFO, "vold", "Failed to ensure dir %s exists",
-                                  appObbSource.c_str());
-            continue;
-        }
-        status = EnsureDirExists(appDataSource, 0770, uid, AID_MEDIA_RW);
-        if (status != OK) {
-            async_safe_format_log(ANDROID_LOG_INFO, "vold", "Failed to ensure dir %s exists",
-                                  appDataSource.c_str());
-            continue;
-        }
-        async_safe_format_log(ANDROID_LOG_INFO, "vold",
-                              "Bind mounting app obb and data directory(%s) for pid %d",
-                              pkg_name.c_str(), pid);
-        auto status1 = BindMount(appObbSource, appObbTarget);
-        // Still bind mount data even obb fails, just slower to access obb dir
-        auto status2 = BindMount(appDataSource, appDataTarget);
-        if (status1 != OK) {
-            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s",
-                                  obbSource.c_str(), obbTarget.c_str(), strerror(-status));
-            continue;
-        }
-        if (status2 != OK) {
-            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s",
-                                  appDataSource.c_str(), appDataTarget.c_str(), strerror(-status));
-            continue;
-        }
     }
     return true;
 }
 
-int VolumeManager::remountAppStorageDirs(userid_t userId) {
-    if (!GetBoolProperty(android::vold::kPropFuse, false)) {
-        return 0;
+static std::string getStorageDirSrc(userid_t userId, const std::string& dirName,
+        const std::string& packageName) {
+    if (IsFilesystemSupported("sdcardfs")) {
+        return StringPrintf("/mnt/runtime/default/emulated/%d/%s/%s",
+                userId, dirName.c_str(), packageName.c_str());
+    } else {
+        return StringPrintf("/mnt/pass_through/%d/emulated/%d/%s/%s",
+                userId, userId, dirName.c_str(), packageName.c_str());
     }
-    LOG(INFO) << "Start remounting app obb and data";
-    pid_t child;
-    if (!(child = fork())) {
-        // Child process
-        if (daemon(0, 0) == -1) {
-            PLOG(FATAL) << "Cannot create daemon";
+}
+
+static std::string getStorageDirTarget(userid_t userId, std::string dirName,
+        std::string packageName) {
+    return StringPrintf("/storage/emulated/%d/%s/%s",
+            userId, dirName.c_str(), packageName.c_str());
+}
+
+// Fork the process and remount storage
+static bool forkAndRemountStorage(int uid, int pid, const std::vector<std::string>& packageNames) {
+    userid_t userId = multiuser_get_user_id(uid);
+    std::string mnt_path = StringPrintf("/proc/%d/ns/mnt", pid);
+    android::base::unique_fd nsFd(
+            TEMP_FAILURE_RETRY(open(mnt_path.c_str(), O_RDONLY | O_CLOEXEC)));
+    if (nsFd == -1) {
+        PLOG(ERROR) << "Unable to open " << mnt_path.c_str();
+        return false;
+    }
+    // Storing both Android/obb and Android/data paths.
+    int size = packageNames.size() * 2;
+
+    std::unique_ptr<std::string[]> sources(new std::string[size]);
+    std::unique_ptr<std::string[]> targets(new std::string[size]);
+    std::unique_ptr<const char*[]> sources_uptr(new const char*[size]);
+    std::unique_ptr<const char*[]> targets_uptr(new const char*[size]);
+    const char** sources_cstr = sources_uptr.get();
+    const char** targets_cstr = targets_uptr.get();
+
+    for (int i = 0; i < size; i += 2) {
+        std::string const& packageName = packageNames[i/2];
+        sources[i] = getStorageDirSrc(userId, "Android/data", packageName);
+        targets[i] = getStorageDirTarget(userId, "Android/data", packageName);
+        sources[i+1] = getStorageDirSrc(userId, "Android/obb", packageName);
+        targets[i+1] = getStorageDirTarget(userId, "Android/obb", packageName);
+
+        sources_cstr[i] = sources[i].c_str();
+        targets_cstr[i] = targets[i].c_str();
+        sources_cstr[i+1] = sources[i+1].c_str();
+        targets_cstr[i+1] = targets[i+1].c_str();
+    }
+
+    for (int i = 0; i < size; i++) {
+        auto status = EnsureDirExists(sources_cstr[i], 0771, AID_MEDIA_RW, AID_MEDIA_RW);
+        if (status != OK) {
+            PLOG(ERROR) << "Failed to create dir: " << sources_cstr[i];
+            return false;
         }
-        // TODO(149548518): Refactor the code so minimize the work after fork to prevent deadlock.
-        if (scanProcProcesses(0, userId, bindMountAppDataObbDir, nullptr)) {
-            // As some forked zygote processes may not setuid and recognized as an app yet, sleep
-            // 3s and try again to catch 'em all.
-            usleep(3 * 1000 * 1000);  // 3s
-            async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Retry remounting app obb");
-            scanProcProcesses(0, userId, bindMountAppDataObbDir, nullptr);
+        status = EnsureDirExists(targets_cstr[i], 0771, AID_MEDIA_RW, AID_MEDIA_RW);
+        if (status != OK) {
+            PLOG(ERROR) << "Failed to create dir: " << targets_cstr[i];
+            return false;
+        }
+    }
+
+    pid_t child;
+    // Fork a child to mount Android/obb android Android/data dirs, as we don't want it to affect
+    // original vold process mount namespace.
+    if (!(child = fork())) {
+        if (remountStorageDirs(nsFd, sources_cstr, targets_cstr, size)) {
             _exit(0);
         } else {
             _exit(1);
         }
     }
+
     if (child == -1) {
         PLOG(ERROR) << "Failed to fork";
-        return -1;
-    } else if (child == 0) {
-        // Parent
-        int stat_loc;
-        for (;;) {
-            if (waitpid(child, &stat_loc, 0) != -1 || errno != EINTR) {
-                break;
-            }
+        return false;
+    } else {
+        int status;
+        if (TEMP_FAILURE_RETRY(waitpid(child, &status, 0)) == -1) {
+            PLOG(ERROR) << "Failed to waitpid: " << child;
+            return false;
+        }
+        if (!WIFEXITED(status)) {
+            PLOG(ERROR) << "Process did not exit normally, status: " << status;
+            return false;
+        }
+        if (WEXITSTATUS(status)) {
+            PLOG(ERROR) << "Process exited with code: " << WEXITSTATUS(status);
+            return false;
         }
     }
+    return true;
+}
+
+int VolumeManager::remountAppStorageDirs(int uid, int pid,
+        const std::vector<std::string>& packageNames) {
+    if (!GetBoolProperty(android::vold::kPropFuse, false)) {
+        return 0;
+    }
+    // Only run the remount if fuse is mounted for that user.
+    userid_t userId = multiuser_get_user_id(uid);
+    bool fuseMounted = false;
+    for (auto& vol : mInternalEmulatedVolumes) {
+        if (vol->getMountUserId() == userId && vol->getState() == VolumeBase::State::kMounted) {
+            auto* emulatedVol = static_cast<android::vold::EmulatedVolume*>(vol.get());
+            if (emulatedVol) {
+                fuseMounted = emulatedVol->isFuseMounted();
+            }
+            break;
+        }
+    }
+    if (fuseMounted) {
+        forkAndRemountStorage(uid, pid, packageNames);
+    }
     return 0;
 }
 
-bool VolumeManager::updateFuseMountedProperty() {
-    if (mFuseMountedUsers.size() == 0) {
-        android::base::SetProperty("vold.fuse_running_users", "");
-        return true;
-    }
-    std::stringstream stream;
-    char const * sep = "";
-    for (const auto& userId : mFuseMountedUsers) {
-        stream << sep;
-        stream << userId;
-        sep = ", ";
-    }
-    return android::base::SetProperty("vold.fuse_running_users", stream.str());
-}
-
-bool VolumeManager::addFuseMountedUser(userid_t userId) {
-    mFuseMountedUsers.insert(userId);
-    return updateFuseMountedProperty();
-}
-
-bool VolumeManager::removeFuseMountedUser(userid_t userId) {
-    mFuseMountedUsers.erase(userId);
-    return updateFuseMountedProperty();
-}
-
 int VolumeManager::reset() {
     // Tear down all existing disks/volumes and start from a blank slate so
     // newly connected framework hears all events.
@@ -975,8 +891,6 @@
     updateVirtualDisk();
     mAddedUsers.clear();
     mStartedUsers.clear();
-    mFuseMountedUsers.clear();
-    updateFuseMountedProperty();
     return 0;
 }
 
@@ -996,8 +910,6 @@
     mInternalEmulatedVolumes.clear();
     mDisks.clear();
     mPendingDisks.clear();
-    mFuseMountedUsers.clear();
-    updateFuseMountedProperty();
     android::vold::sSleepOnUnmount = true;
     return 0;
 }
diff --git a/VolumeManager.h b/VolumeManager.h
index bf05dcf..b83871e 100644
--- a/VolumeManager.h
+++ b/VolumeManager.h
@@ -118,10 +118,7 @@
     int setPrimary(const std::shared_ptr<android::vold::VolumeBase>& vol);
 
     int remountUid(uid_t uid, int32_t remountMode);
-    int remountAppStorageDirs(userid_t userId);
-
-    bool addFuseMountedUser(userid_t userId);
-    bool removeFuseMountedUser(userid_t userId);
+    int remountAppStorageDirs(int uid, int pid, const std::vector<std::string>& packageNames);
 
     /* Reset all internal state, typically during framework boot */
     int reset();
@@ -230,9 +227,6 @@
     int mNextObbId;
     int mNextStubId;
     bool mSecureKeyguardShowing;
-
-    // Set of all user id that fuse is ready to use.
-    std::unordered_set<userid_t> mFuseMountedUsers;
 };
 
 #endif
diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl
index f1ada6c..1d5657f 100644
--- a/binder/android/os/IVold.aidl
+++ b/binder/android/os/IVold.aidl
@@ -53,6 +53,7 @@
                      IVoldTaskListener listener);
 
     void remountUid(int uid, int remountMode);
+    void remountAppStorageDirs(int uid, int pid, in @utf8InCpp String[] packageNames);
 
     void setupAppDir(@utf8InCpp String path, int appUid);
     void fixupAppDir(@utf8InCpp String path, int appUid);
diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp
index 1391685..e411b33 100644
--- a/model/EmulatedVolume.cpp
+++ b/model/EmulatedVolume.cpp
@@ -22,6 +22,7 @@
 
 #include <android-base/logging.h>
 #include <android-base/properties.h>
+#include <android-base/scopeguard.h>
 #include <android-base/stringprintf.h>
 #include <cutils/fs.h>
 #include <private/android_filesystem_config.h>
@@ -76,13 +77,15 @@
 }
 
 // Creates a bind mount from source to target
-static status_t doFuseBindMount(const std::string& source, const std::string& target) {
+static status_t doFuseBindMount(const std::string& source, const std::string& target,
+                                std::list<std::string>& pathsToUnmount) {
     LOG(INFO) << "Bind mounting " << source << " on " << target;
     auto status = BindMount(source, target);
     if (status != OK) {
         return status;
     }
     LOG(INFO) << "Bind mounted " << source << " on " << target;
+    pathsToUnmount.push_front(target);
     return OK;
 }
 
@@ -90,6 +93,21 @@
     std::string androidSource;
     std::string label = getLabel();
     int userId = getMountUserId();
+    std::list<std::string> pathsToUnmount;
+
+    auto unmounter = [&]() {
+        LOG(INFO) << "mountFuseBindMounts() unmount scope_guard running";
+        for (const auto& path : pathsToUnmount) {
+            LOG(INFO) << "Unmounting " << path;
+            auto status = UnmountTree(path);
+            if (status != OK) {
+                LOG(INFO) << "Failed to unmount " << path;
+            } else {
+                LOG(INFO) << "Unmounted " << path;
+            }
+        }
+    };
+    auto unmount_guard = android::base::make_scope_guard(unmounter);
 
     if (mUseSdcardFs) {
         androidSource = StringPrintf("/mnt/runtime/default/%s/%d/Android", label.c_str(), userId);
@@ -101,43 +119,39 @@
     // When app data isolation is enabled, obb/ will be mounted per app, otherwise we should
     // bind mount the whole Android/ to speed up reading.
     if (!mAppDataIsolationEnabled) {
-        std::string androidTarget(
-            StringPrintf("/mnt/user/%d/%s/%d/Android", userId, label.c_str(), userId));
-        status = doFuseBindMount(androidSource, androidTarget);
+        std::string androidDataSource = StringPrintf("%s/data", androidSource.c_str());
+        std::string androidDataTarget(
+                StringPrintf("/mnt/user/%d/%s/%d/Android/data", userId, label.c_str(), userId));
+        status = doFuseBindMount(androidDataSource, androidDataTarget, pathsToUnmount);
+        if (status != OK) {
+            return status;
+        }
+
+        std::string androidObbSource = StringPrintf("%s/obb", androidSource.c_str());
+        std::string androidObbTarget(
+                StringPrintf("/mnt/user/%d/%s/%d/Android/obb", userId, label.c_str(), userId));
+        status = doFuseBindMount(androidObbSource, androidObbTarget, pathsToUnmount);
+        if (status != OK) {
+            return status;
+        }
     }
 
-    if (status != OK) {
-        return status;
-    }
     // Installers get the same view as all other apps, with the sole exception that the
     // OBB dirs (Android/obb) are writable to them. On sdcardfs devices, this requires
     // a special bind mount, since app-private and OBB dirs share the same GID, but we
     // only want to give access to the latter.
-    if (!mUseSdcardFs) {
-        return OK;
-    }
-    std::string installerSource(
-            StringPrintf("/mnt/runtime/write/%s/%d/Android/obb", label.c_str(), userId));
-    std::string installerTarget(
-            StringPrintf("/mnt/installer/%d/%s/%d/Android/obb", userId, label.c_str(), userId));
+    if (mUseSdcardFs) {
+        std::string installerSource(
+                StringPrintf("/mnt/runtime/write/%s/%d/Android/obb", label.c_str(), userId));
+        std::string installerTarget(
+                StringPrintf("/mnt/installer/%d/%s/%d/Android/obb", userId, label.c_str(), userId));
 
-    status = doFuseBindMount(installerSource, installerTarget);
-    if (status != OK) {
-        return status;
-    }
-
-    if (mAppDataIsolationEnabled) {
-        // Starting from now, fuse is running, and zygote will bind app obb & data directory
-        if (!VolumeManager::Instance()->addFuseMountedUser(userId)) {
-            return UNKNOWN_ERROR;
+        status = doFuseBindMount(installerSource, installerTarget, pathsToUnmount);
+        if (status != OK) {
+            return status;
         }
-
-        // As all new processes created by zygote will bind app obb data directory, we just need
-        // to have a snapshot of all existing processes and see if any existing process needs to
-        // remount obb data directory.
-        VolumeManager::Instance()->remountAppStorageDirs(userId);
     }
-
+    unmount_guard.Disable();
     return OK;
 }
 
@@ -161,16 +175,50 @@
         std::string appObbDir(StringPrintf("%s/%d/Android/obb", getPath().c_str(), userId));
         KillProcessesWithMountPrefix(appObbDir);
     } else {
-        std::string androidTarget(
-                StringPrintf("/mnt/user/%d/%s/%d/Android", userId, label.c_str(), userId));
+        std::string androidDataTarget(
+                StringPrintf("/mnt/user/%d/%s/%d/Android/data", userId, label.c_str(), userId));
 
-        LOG(INFO) << "Unmounting " << androidTarget;
-        auto status = UnmountTree(androidTarget);
+        LOG(INFO) << "Unmounting " << androidDataTarget;
+        auto status = UnmountTree(androidDataTarget);
         if (status != OK) {
             return status;
         }
-        LOG(INFO) << "Unmounted " << androidTarget;
+        LOG(INFO) << "Unmounted " << androidDataTarget;
+
+        std::string androidObbTarget(
+                StringPrintf("/mnt/user/%d/%s/%d/Android/obb", userId, label.c_str(), userId));
+
+        LOG(INFO) << "Unmounting " << androidObbTarget;
+        status = UnmountTree(androidObbTarget);
+        if (status != OK) {
+            return status;
+        }
+        LOG(INFO) << "Unmounted " << androidObbTarget;
     }
+    return OK;
+}
+
+status_t EmulatedVolume::unmountSdcardFs() {
+    if (!mUseSdcardFs || getMountUserId() != 0) {
+        // For sdcardfs, only unmount for user 0, since user 0 will always be running
+        // and the paths don't change for different users.
+        return OK;
+    }
+
+    ForceUnmount(mSdcardFsDefault);
+    ForceUnmount(mSdcardFsRead);
+    ForceUnmount(mSdcardFsWrite);
+    ForceUnmount(mSdcardFsFull);
+
+    rmdir(mSdcardFsDefault.c_str());
+    rmdir(mSdcardFsRead.c_str());
+    rmdir(mSdcardFsWrite.c_str());
+    rmdir(mSdcardFsFull.c_str());
+
+    mSdcardFsDefault.clear();
+    mSdcardFsRead.clear();
+    mSdcardFsWrite.clear();
+    mSdcardFsFull.clear();
 
     return OK;
 }
@@ -245,7 +293,15 @@
         TEMP_FAILURE_RETRY(waitpid(sdcardFsPid, nullptr, 0));
         sdcardFsPid = 0;
     }
+
     if (isFuse && isVisible) {
+        // Make sure we unmount sdcardfs if we bail out with an error below
+        auto sdcardfs_unmounter = [&]() {
+            LOG(INFO) << "sdcardfs_unmounter scope_guard running";
+            unmountSdcardFs();
+        };
+        auto sdcardfs_guard = android::base::make_scope_guard(sdcardfs_unmounter);
+
         LOG(INFO) << "Mounting emulated fuse volume";
         android::base::unique_fd fd;
         int user_id = getMountUserId();
@@ -265,13 +321,21 @@
         }
 
         mFuseMounted = true;
+        auto fuse_unmounter = [&]() {
+            LOG(INFO) << "fuse_unmounter scope_guard running";
+            fd.reset();
+            if (UnmountUserFuse(user_id, getInternalPath(), label) != OK) {
+                PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
+            }
+            mFuseMounted = false;
+        };
+        auto fuse_guard = android::base::make_scope_guard(fuse_unmounter);
+
         auto callback = getMountCallback();
         if (callback) {
             bool is_ready = false;
             callback->onVolumeChecking(std::move(fd), getPath(), getInternalPath(), &is_ready);
             if (!is_ready) {
-                fd.reset();
-                doUnmount();
                 return -EIO;
             }
         }
@@ -279,10 +343,12 @@
         // Only do the bind-mounts when we know for sure the FUSE daemon can resolve the path.
         res = mountFuseBindMounts();
         if (res != OK) {
-            fd.reset();
-            doUnmount();
+            return res;
         }
-        return res;
+
+        // All mounts where successful, disable scope guards
+        sdcardfs_guard.Disable();
+        fuse_guard.Disable();
     }
 
     return OK;
@@ -308,15 +374,10 @@
     if (mFuseMounted) {
         std::string label = getLabel();
 
-        // Update fuse mounted record
-        if (mAppDataIsolationEnabled &&
-                !VolumeManager::Instance()->removeFuseMountedUser(userId)) {
-            return UNKNOWN_ERROR;
-        }
-
         // Ignoring unmount return status because we do want to try to unmount
         // the rest cleanly.
         unmountFuseBindMounts();
+
         if (UnmountUserFuse(userId, getInternalPath(), label) != OK) {
             PLOG(INFO) << "UnmountUserFuse failed on emulated fuse volume";
             return -errno;
@@ -324,28 +385,8 @@
 
         mFuseMounted = false;
     }
-    if (getMountUserId() != 0 || !mUseSdcardFs) {
-        // For sdcardfs, only unmount for user 0, since user 0 will always be running
-        // and the paths don't change for different users.
-        return OK;
-    }
 
-    ForceUnmount(mSdcardFsDefault);
-    ForceUnmount(mSdcardFsRead);
-    ForceUnmount(mSdcardFsWrite);
-    ForceUnmount(mSdcardFsFull);
-
-    rmdir(mSdcardFsDefault.c_str());
-    rmdir(mSdcardFsRead.c_str());
-    rmdir(mSdcardFsWrite.c_str());
-    rmdir(mSdcardFsFull.c_str());
-
-    mSdcardFsDefault.clear();
-    mSdcardFsRead.clear();
-    mSdcardFsWrite.clear();
-    mSdcardFsFull.clear();
-
-    return OK;
+    return unmountSdcardFs();
 }
 
 std::string EmulatedVolume::getRootPath() const {
diff --git a/model/EmulatedVolume.h b/model/EmulatedVolume.h
index 12d01ec..1d2385d 100644
--- a/model/EmulatedVolume.h
+++ b/model/EmulatedVolume.h
@@ -41,12 +41,14 @@
     EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, int userId);
     virtual ~EmulatedVolume();
     std::string getRootPath() const override;
+    bool isFuseMounted() const { return mFuseMounted; }
 
   protected:
     status_t doMount() override;
     status_t doUnmount() override;
 
   private:
+    status_t unmountSdcardFs();
     status_t mountFuseBindMounts();
     status_t unmountFuseBindMounts();
 
diff --git a/tests/Android.bp b/tests/Android.bp
index 4731d0a..b90de3a 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -7,7 +7,9 @@
 
     srcs: [
         "Utils_test.cpp",
+        "VoldNativeServiceValidation_test.cpp",
         "cryptfs_test.cpp",
     ],
     static_libs: ["libvold"],
+    shared_libs: ["libbinder"]
 }
diff --git a/tests/VoldNativeServiceValidation_test.cpp b/tests/VoldNativeServiceValidation_test.cpp
new file mode 100644
index 0000000..0f87937
--- /dev/null
+++ b/tests/VoldNativeServiceValidation_test.cpp
@@ -0,0 +1,51 @@
+/*
+ * 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.
+ */
+
+#include "../VoldNativeServiceValidation.h"
+
+#include <gtest/gtest.h>
+
+#include <string_view>
+
+using namespace std::literals;
+
+namespace android::vold {
+
+class VoldServiceValidationTest : public testing::Test {};
+
+TEST_F(VoldServiceValidationTest, CheckArgumentPathTest) {
+    EXPECT_TRUE(CheckArgumentPath("/").isOk());
+    EXPECT_TRUE(CheckArgumentPath("/1/2").isOk());
+    EXPECT_TRUE(CheckArgumentPath("/1/2/").isOk());
+    EXPECT_TRUE(CheckArgumentPath("/1/2/./3").isOk());
+    EXPECT_TRUE(CheckArgumentPath("/1/2/./3/.").isOk());
+    EXPECT_TRUE(CheckArgumentPath(
+                        "/very long path with some/ spaces and quite/ uncommon names /in\1 it/")
+                        .isOk());
+
+    EXPECT_FALSE(CheckArgumentPath("").isOk());
+    EXPECT_FALSE(CheckArgumentPath("relative/path").isOk());
+    EXPECT_FALSE(CheckArgumentPath("../data/..").isOk());
+    EXPECT_FALSE(CheckArgumentPath("/../data/..").isOk());
+    EXPECT_FALSE(CheckArgumentPath("/data/../system").isOk());
+    EXPECT_FALSE(CheckArgumentPath("/data/..trick/../system").isOk());
+    EXPECT_FALSE(CheckArgumentPath("/data/..").isOk());
+    EXPECT_FALSE(CheckArgumentPath("/data/././../apex").isOk());
+    EXPECT_FALSE(CheckArgumentPath(std::string("/data/strange\0one"sv)).isOk());
+    EXPECT_FALSE(CheckArgumentPath(std::string("/data/strange\ntwo"sv)).isOk());
+}
+
+}  // namespace android::vold