diff --git a/Utils.cpp b/Utils.cpp index ce1f777..c0b7f01 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -235,7 +236,7 @@ static status_t readMetadata(const std::string& path, std::string* fsType, std:: cmd.push_back(path); std::vector 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 @@ status_t ReadMetadataUntrusted(const std::string& path, std::string* fsType, std return readMetadata(path, fsType, fsUuid, fsLabel, true); } -status_t ForkExecvp(const std::vector& args) { - return ForkExecvp(args, nullptr); -} - -status_t ForkExecvp(const std::vector& args, security_context_t context) { - std::lock_guard 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 ConvertToArgv(const std::vector& args) { + std::vector 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()); } + argv.emplace_back(nullptr); + return argv; +} - if (context) { - if (setexeccon(context)) { - LOG(ERROR) << "Failed to setexeccon"; - abort(); - } +static status_t ReadLinesFromFdAndLog(std::vector* output, + android::base::unique_fd ufd) { + std::unique_ptr fp(android::base::Fdopen(std::move(ufd), "r"), fclose); + if (!fp) { + PLOG(ERROR) << "fdopen in ReadLinesFromFdAndLog"; + return -errno; } - status_t res = android_fork_execvp(argc, argv, NULL, false, true); - if (context) { - if (setexeccon(nullptr)) { - LOG(ERROR) << "Failed to setexeccon"; - abort(); - } + if (output) output->clear(); + char line[1024]; + while (fgets(line, sizeof(line), fp.get()) != nullptr) { + LOG(DEBUG) << line; + if (output) output->emplace_back(line); } - - free(argv); - return res; -} - -status_t ForkExecvp(const std::vector& args, std::vector& output) { - return ForkExecvp(args, output, nullptr); + return OK; } -status_t ForkExecvp(const std::vector& args, std::vector& output, +status_t ForkExecvp(const std::vector& args, std::vector* output, security_context_t context) { - std::lock_guard 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]; - } + auto argv = ConvertToArgv(args); + + int fd[2]; + if (pipe2(fd, O_CLOEXEC) != 0) { + PLOG(ERROR) << "pipe2 in ForkExecvp"; + return -errno; } - output.clear(); + android::base::unique_fd pipe_read(fd[0]); + android::base::unique_fd pipe_write(fd[1]); - if (context) { - if (setexeccon(context)) { - LOG(ERROR) << "Failed to setexeccon"; - abort(); + pid_t pid = fork(); + if (pid == 0) { + 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(); + pipe_read.reset(); + if (pipe_write.get() != STDOUT_FILENO) { + dup2(pipe_write.get(), STDOUT_FILENO); + pipe_write.reset(); } + execvp(argv[0], const_cast(argv.data())); + PLOG(ERROR) << "Failed to exec"; + _exit(EXIT_FAILURE); + } + if (pid == -1) { + PLOG(ERROR) << "fork in ForkExecvp"; + return -errno; } - if (!fp) { - PLOG(ERROR) << "Failed to popen " << cmd; + 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; } - char line[1024]; - while (fgets(line, sizeof(line), fp) != nullptr) { - LOG(DEBUG) << line; - output.push_back(std::string(line)); + if (!WIFEXITED(status)) { + LOG(ERROR) << "Process did not exit normally, status: " << status; + return -ECHILD; } - if (pclose(fp) != 0) { - PLOG(ERROR) << "Failed to pclose " << cmd; - return -errno; + if (WEXITSTATUS(status)) { + LOG(ERROR) << "Process exited with code: " << WEXITSTATUS(status); + return WEXITSTATUS(status); } - return OK; } pid_t ForkExecvpAsync(const std::vector& 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 @@ pid_t ForkExecvpAsync(const std::vector& args) { close(STDOUT_FILENO); close(STDERR_FILENO); - if (execvp(argv[0], argv)) { - PLOG(ERROR) << "Failed to exec"; - } - - _exit(1); + execvp(argv[0], const_cast(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 @@ status_t ReadMetadataUntrusted(const std::string& path, std::string* fsType, std std::string* fsLabel); /* Returns either WEXITSTATUS() status, or a negative errno */ -status_t ForkExecvp(const std::vector& args); -status_t ForkExecvp(const std::vector& args, security_context_t context); - -status_t ForkExecvp(const std::vector& args, std::vector& output); -status_t ForkExecvp(const std::vector& args, std::vector& output, - security_context_t context); +status_t ForkExecvp(const std::vector& args, std::vector* output = nullptr, + security_context_t context = nullptr); pid_t ForkExecvpAsync(const std::vector& 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 @@ status_t Check(const std::string& source) { 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 @@ status_t Check(const std::string& source, const std::string& target) { 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 @@ status_t Check(const std::string& source) { 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 @@ status_t Check(const std::string& source) { 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 @@ status_t Disk::readPartitions() { cmd.push_back(mDevPath); std::vector output; - status_t res = ForkExecvp(cmd, output); + status_t res = ForkExecvp(cmd, &output); if (res != OK) { LOG(WARNING) << "sgdisk failed to scan " << mDevPath;