patchpanel: Resolve deadlock in reading stdout pipe
Currently, MinijailedProcessRunner reads the stdout pipe after the
execution of subprocess is done. This may cause a deadlock when the
buffer size of the pipe cannot accommodate the whole output: subprocess
is stuck at writing to the pipe since the buffer is full, while
patchpanel is waiting for the execution finish.
This patch resolves this deadlock by moving the logic for reading the
pipe before waitpid().
Note that after this patch, patchpanel may still be not responsive if
the subprocess itself running by this runner is stuck, or takes too much
time.
BUG=b:175842822
TEST=deployed on octopus, add 4000 rules in the mangle table by `for i
in {1..4000}; do iptables -t mangle -A tx_eth0 -j ACCEPT; done`,
invoke the traffic counters by `in {1..1000}; do dbus-send --system
--dest=org.chromium.PatchPanel --print-reply /org/chromium/PatchPanel
org.chromium.PatchPanel.GetTrafficCounters; done` (patchpanel code is
modified to accept requests with no parameter), after that, checked
that no fd is leaked and no suspicious log in /var/log/net.log
Change-Id: I740fa40b83c39ff4510abe7f4e93afe00ee43217
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2593504
Tested-by: Jie Jiang <jiejiang@chromium.org>
Reviewed-by: Garrick Evans <garrick@chromium.org>
Reviewed-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Jie Jiang <jiejiang@chromium.org>
diff --git a/patchpanel/minijailed_process_runner.cc b/patchpanel/minijailed_process_runner.cc
index 9b1f29d..d0cc785 100644
--- a/patchpanel/minijailed_process_runner.cc
+++ b/patchpanel/minijailed_process_runner.cc
@@ -8,6 +8,7 @@
#include <sys/wait.h>
#include <utility>
+#include <base/files/scoped_file.h>
#include <base/logging.h>
#include <base/posix/eintr_wrapper.h>
#include <base/strings/string_number_conversions.h>
@@ -38,19 +39,25 @@
constexpr char kSysctlPath[] = "/usr/sbin/sysctl";
// An empty string will be returned if read fails.
-std::string ReadBlockingFDToString(int fd) {
+std::string ReadBlockingFDToStringAndClose(base::ScopedFD fd) {
+ if (!fd.is_valid()) {
+ LOG(ERROR) << "Invalid fd";
+ return "";
+ }
+
static constexpr int kBufSize = 2048;
char buf[kBufSize] = {0};
std::string output;
while (true) {
- ssize_t cnt = HANDLE_EINTR(read(fd, buf, kBufSize));
+ ssize_t cnt = HANDLE_EINTR(read(fd.get(), buf, kBufSize));
if (cnt == -1) {
PLOG(ERROR) << __func__ << " failed";
- return {};
+ return "";
}
- if (cnt == 0)
+ if (cnt == 0) {
return output;
+ }
output.append({buf, static_cast<size_t>(cnt)});
}
@@ -69,7 +76,7 @@
brillo::Minijail* mj,
minijail* jail,
bool log_failures,
- int* fd_stdout) {
+ std::string* output) {
std::vector<char*> args;
for (const auto& arg : argv) {
args.push_back(const_cast<char*>(arg.c_str()));
@@ -77,9 +84,15 @@
args.push_back(nullptr);
pid_t pid;
- int status = 0;
+ int fd_stdout = -1;
+ int* stdout_p = output ? &fd_stdout : nullptr;
bool ran = mj->RunPipesAndDestroy(jail, args, &pid, nullptr /*stdin*/,
- fd_stdout, nullptr /*stderr*/);
+ stdout_p, nullptr /*stderr*/);
+ if (output) {
+ *output = ReadBlockingFDToStringAndClose(base::ScopedFD(fd_stdout));
+ }
+
+ int status = 0;
if (ran) {
ran = syscall_->WaitPID(pid, &status) == pid;
}
@@ -104,19 +117,7 @@
int MinijailedProcessRunner::RunSync(const std::vector<std::string>& argv,
bool log_failures,
std::string* output) {
- if (!output) {
- return RunSyncDestroy(argv, mj_, mj_->New(), log_failures, nullptr);
- }
-
- int fd_stdout = -1;
- int ret = RunSyncDestroy(argv, mj_, mj_->New(), log_failures, &fd_stdout);
- if (ret == 0 && fd_stdout > 0) {
- *output = ReadBlockingFDToString(fd_stdout);
- }
- if (fd_stdout > 0) {
- close(fd_stdout);
- }
- return ret;
+ return RunSyncDestroy(argv, mj_, mj_->New(), log_failures, output);
}
void EnterChildProcessJail() {
diff --git a/patchpanel/minijailed_process_runner.h b/patchpanel/minijailed_process_runner.h
index b0de410..b52ea68 100644
--- a/patchpanel/minijailed_process_runner.h
+++ b/patchpanel/minijailed_process_runner.h
@@ -111,7 +111,7 @@
brillo::Minijail* mj,
minijail* jail,
bool log_failures,
- int* fd_stdout);
+ std::string* output);
brillo::Minijail* mj_;
