Refactor ForkExecvp to improve locking behaviour
Do our own fork/exec rather than using a library. This leads to
many improvements:
- unite the output recording path with the other path
- never concatenate arguments with spaces
- never use the shell
- move setexeccon after fork, so we don't need to take the lock
- general code refactor while we're there
My tests:
- Ensure Marlin device boots and vold_prepare_subdirs is called
successfully
- Try adb shell sm set-virtual-disk true, see that eg sgdisk output is
logged.
weilongping@huawei.com's tests:
- unlock a user's de and ce directory;
- connect to a OTG storage device or a sdcard and ensure the mount logic be successful
Bug: 26735063
Bug: 113796163
Test: details in commit
Change-Id: I0976413529d7cbeebf5b8649660a385f9b036f04
diff --git a/Utils.cpp b/Utils.cpp
index ce1f777..c0b7f01 100644
--- a/Utils.cpp
+++ b/Utils.cpp
@@ -25,6 +25,7 @@
#include <android-base/properties.h>
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
+#include <android-base/unique_fd.h>
#include <cutils/fs.h>
#include <logwrap/logwrap.h>
#include <private/android_filesystem_config.h>
@@ -235,7 +236,7 @@
cmd.push_back(path);
std::vector<std::string> output;
- status_t res = ForkExecvp(cmd, output, untrusted ? sBlkidUntrustedContext : sBlkidContext);
+ status_t res = ForkExecvp(cmd, &output, untrusted ? sBlkidUntrustedContext : sBlkidContext);
if (res != OK) {
LOG(WARNING) << "blkid failed to identify " << path;
return res;
@@ -261,101 +262,93 @@
return readMetadata(path, fsType, fsUuid, fsLabel, true);
}
-status_t ForkExecvp(const std::vector<std::string>& args) {
- return ForkExecvp(args, nullptr);
-}
-
-status_t ForkExecvp(const std::vector<std::string>& args, security_context_t context) {
- std::lock_guard<std::mutex> lock(kSecurityLock);
- size_t argc = args.size();
- char** argv = (char**)calloc(argc, sizeof(char*));
- for (size_t i = 0; i < argc; i++) {
- argv[i] = (char*)args[i].c_str();
- if (i == 0) {
- LOG(DEBUG) << args[i];
+static std::vector<const char*> ConvertToArgv(const std::vector<std::string>& args) {
+ std::vector<const char*> argv;
+ argv.reserve(args.size() + 1);
+ for (const auto& arg : args) {
+ if (argv.empty()) {
+ LOG(DEBUG) << arg;
} else {
- LOG(DEBUG) << " " << args[i];
+ LOG(DEBUG) << " " << arg;
}
+ argv.emplace_back(arg.data());
}
-
- if (context) {
- if (setexeccon(context)) {
- LOG(ERROR) << "Failed to setexeccon";
- abort();
- }
- }
- status_t res = android_fork_execvp(argc, argv, NULL, false, true);
- if (context) {
- if (setexeccon(nullptr)) {
- LOG(ERROR) << "Failed to setexeccon";
- abort();
- }
- }
-
- free(argv);
- return res;
+ argv.emplace_back(nullptr);
+ return argv;
}
-status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>& output) {
- return ForkExecvp(args, output, nullptr);
-}
-
-status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>& output,
- security_context_t context) {
- std::lock_guard<std::mutex> lock(kSecurityLock);
- std::string cmd;
- for (size_t i = 0; i < args.size(); i++) {
- cmd += args[i] + " ";
- if (i == 0) {
- LOG(DEBUG) << args[i];
- } else {
- LOG(DEBUG) << " " << args[i];
- }
- }
- output.clear();
-
- if (context) {
- if (setexeccon(context)) {
- LOG(ERROR) << "Failed to setexeccon";
- abort();
- }
- }
- FILE* fp = popen(cmd.c_str(), "r"); // NOLINT
- if (context) {
- if (setexeccon(nullptr)) {
- LOG(ERROR) << "Failed to setexeccon";
- abort();
- }
- }
-
+static status_t ReadLinesFromFdAndLog(std::vector<std::string>* output,
+ android::base::unique_fd ufd) {
+ std::unique_ptr<FILE, int (*)(FILE*)> fp(android::base::Fdopen(std::move(ufd), "r"), fclose);
if (!fp) {
- PLOG(ERROR) << "Failed to popen " << cmd;
+ PLOG(ERROR) << "fdopen in ReadLinesFromFdAndLog";
return -errno;
}
+ if (output) output->clear();
char line[1024];
- while (fgets(line, sizeof(line), fp) != nullptr) {
+ while (fgets(line, sizeof(line), fp.get()) != nullptr) {
LOG(DEBUG) << line;
- output.push_back(std::string(line));
+ if (output) output->emplace_back(line);
}
- if (pclose(fp) != 0) {
- PLOG(ERROR) << "Failed to pclose " << cmd;
+ return OK;
+}
+
+status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>* output,
+ security_context_t context) {
+ auto argv = ConvertToArgv(args);
+
+ int fd[2];
+ if (pipe2(fd, O_CLOEXEC) != 0) {
+ PLOG(ERROR) << "pipe2 in ForkExecvp";
+ return -errno;
+ }
+ android::base::unique_fd pipe_read(fd[0]);
+ android::base::unique_fd pipe_write(fd[1]);
+
+ pid_t pid = fork();
+ if (pid == 0) {
+ if (context) {
+ if (setexeccon(context)) {
+ LOG(ERROR) << "Failed to setexeccon";
+ abort();
+ }
+ }
+ pipe_read.reset();
+ if (pipe_write.get() != STDOUT_FILENO) {
+ dup2(pipe_write.get(), STDOUT_FILENO);
+ pipe_write.reset();
+ }
+ execvp(argv[0], const_cast<char**>(argv.data()));
+ PLOG(ERROR) << "Failed to exec";
+ _exit(EXIT_FAILURE);
+ }
+ if (pid == -1) {
+ PLOG(ERROR) << "fork in ForkExecvp";
return -errno;
}
+ pipe_write.reset();
+ auto st = ReadLinesFromFdAndLog(output, std::move(pipe_read));
+ if (st != 0) return st;
+
+ int status;
+ if (waitpid(pid, &status, 0) == -1) {
+ PLOG(ERROR) << "waitpid in ForkExecvp";
+ return -errno;
+ }
+ if (!WIFEXITED(status)) {
+ LOG(ERROR) << "Process did not exit normally, status: " << status;
+ return -ECHILD;
+ }
+ if (WEXITSTATUS(status)) {
+ LOG(ERROR) << "Process exited with code: " << WEXITSTATUS(status);
+ return WEXITSTATUS(status);
+ }
return OK;
}
pid_t ForkExecvpAsync(const std::vector<std::string>& args) {
- size_t argc = args.size();
- char** argv = (char**)calloc(argc + 1, sizeof(char*));
- for (size_t i = 0; i < argc; i++) {
- argv[i] = (char*)args[i].c_str();
- if (i == 0) {
- LOG(DEBUG) << args[i];
- } else {
- LOG(DEBUG) << " " << args[i];
- }
- }
+ auto argv = ConvertToArgv(args);
pid_t pid = fork();
if (pid == 0) {
@@ -363,18 +356,14 @@
close(STDOUT_FILENO);
close(STDERR_FILENO);
- if (execvp(argv[0], argv)) {
- PLOG(ERROR) << "Failed to exec";
- }
-
- _exit(1);
+ execvp(argv[0], const_cast<char**>(argv.data()));
+ PLOG(ERROR) << "exec in ForkExecvpAsync";
+ _exit(EXIT_FAILURE);
}
-
if (pid == -1) {
- PLOG(ERROR) << "Failed to exec";
+ PLOG(ERROR) << "fork in ForkExecvpAsync";
+ return -1;
}
-
- free(argv);
return pid;
}
diff --git a/Utils.h b/Utils.h
index 4d3522a..7976302 100644
--- a/Utils.h
+++ b/Utils.h
@@ -68,12 +68,8 @@
std::string* fsLabel);
/* Returns either WEXITSTATUS() status, or a negative errno */
-status_t ForkExecvp(const std::vector<std::string>& args);
-status_t ForkExecvp(const std::vector<std::string>& args, security_context_t context);
-
-status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>& output);
-status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>& output,
- security_context_t context);
+status_t ForkExecvp(const std::vector<std::string>& args, std::vector<std::string>* output = nullptr,
+ security_context_t context = nullptr);
pid_t ForkExecvpAsync(const std::vector<std::string>& args);
diff --git a/fs/Exfat.cpp b/fs/Exfat.cpp
index 5c15075..c624eb9 100644
--- a/fs/Exfat.cpp
+++ b/fs/Exfat.cpp
@@ -43,7 +43,7 @@
cmd.push_back(kFsckPath);
cmd.push_back(source);
- int rc = ForkExecvp(cmd, sFsckUntrustedContext);
+ int rc = ForkExecvp(cmd, nullptr, sFsckUntrustedContext);
if (rc == 0) {
LOG(INFO) << "Check OK";
return 0;
diff --git a/fs/Ext4.cpp b/fs/Ext4.cpp
index 806cfd1..0059233 100644
--- a/fs/Ext4.cpp
+++ b/fs/Ext4.cpp
@@ -117,7 +117,7 @@
cmd.push_back(c_source);
// ext4 devices are currently always trusted
- return ForkExecvp(cmd, sFsckContext);
+ return ForkExecvp(cmd, nullptr, sFsckContext);
}
return 0;
diff --git a/fs/F2fs.cpp b/fs/F2fs.cpp
index c6e0f52..9517dc9 100644
--- a/fs/F2fs.cpp
+++ b/fs/F2fs.cpp
@@ -48,7 +48,7 @@
cmd.push_back(source);
// f2fs devices are currently always trusted
- return ForkExecvp(cmd, sFsckContext);
+ return ForkExecvp(cmd, nullptr, sFsckContext);
}
status_t Mount(const std::string& source, const std::string& target) {
diff --git a/fs/Vfat.cpp b/fs/Vfat.cpp
index 7b833d1..14c42d6 100644
--- a/fs/Vfat.cpp
+++ b/fs/Vfat.cpp
@@ -67,7 +67,7 @@
cmd.push_back(source);
// Fat devices are currently always untrusted
- rc = ForkExecvp(cmd, sFsckUntrustedContext);
+ rc = ForkExecvp(cmd, nullptr, sFsckUntrustedContext);
if (rc < 0) {
LOG(ERROR) << "Filesystem check failed due to logwrap error";
diff --git a/model/Disk.cpp b/model/Disk.cpp
index 3d25e4c..1568ba4 100644
--- a/model/Disk.cpp
+++ b/model/Disk.cpp
@@ -341,7 +341,7 @@
cmd.push_back(mDevPath);
std::vector<std::string> output;
- status_t res = ForkExecvp(cmd, output);
+ status_t res = ForkExecvp(cmd, &output);
if (res != OK) {
LOG(WARNING) << "sgdisk failed to scan " << mDevPath;