debugd: Close bound FDs when debugd is done with them
Commit 476d59a ("Fix Process::BindFd when passing the same fd
to the child.") in libbrillo changed to not close a FD in
the parent process after fork if the FD was bound. As a result
it causes FD leaks in several places in debugd.
For example, chrome://tracing is broken because the pipe FD is
not closed after debugd handles kSystraceStop DBus call. Another
exmaple is the "Store Logs" in chrome://net-internals/#chromeos
where the logs are stored but debugd never closes the output FD.
We should close the bound FDs in the parent process when debugd
is done with them. This patch overrides BindFd() in SandboxedProcess
to record all the bound FDs and close all the bound FDs in the
destructor. The only place that SandboxedProcess is not used is
in DebugLogsTool::GetDebugLogs(), where the FD is closed explicitly
after the subprocess finishes.
Also the process that runs `systrace.sh stop` does not need to
be ProcessWithOutput, so change it to SandboxedProcess.
BUG=chromium:613122
TEST=On Chrome OS: chrome://tracing
start a trace, be sure to enable "System tracing"
stop trace
=> Chrome successfully imports trace data
(before this patch, Chrome would hang, waiting for debugd to respond)
TEST=On Chrome OS: chrome://net-internals/#chromeos
click on "Store Debug Logs" and wait for logs to be created
run `lsof -p $(pgrep debugd)`
=> no FD leak in debugd
(before this patch, the debugd would leave the logs output file opened)
Change-Id: I78ca534dc654e4dbc79d37a5e0278e2eb18c0e4e
Signed-off-by: Ricky Liang <jcliang@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/346762
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
(cherry picked from commit 1ef73e543200fb7150d2bfb223f4a398067e8599)
Reviewed-on: https://chromium-review.googlesource.com/348120
diff --git a/debugd/src/debug_logs_tool.cc b/debugd/src/debug_logs_tool.cc
index 9bf0c48..b4d1087 100644
--- a/debugd/src/debug_logs_tool.cc
+++ b/debugd/src/debug_logs_tool.cc
@@ -25,6 +25,7 @@
p.AddArg(kSystemLogs);
p.BindFd(fd.get(), STDOUT_FILENO);
p.Run();
+ close(fd.get());
}
} // namespace debugd
diff --git a/debugd/src/sandboxed_process.cc b/debugd/src/sandboxed_process.cc
index a18083d..80438e8 100644
--- a/debugd/src/sandboxed_process.cc
+++ b/debugd/src/sandboxed_process.cc
@@ -28,6 +28,11 @@
group_(kDefaultGroup) {
}
+SandboxedProcess::~SandboxedProcess() {
+ for (const auto& fd : bound_fds_)
+ close(fd);
+}
+
// static
bool SandboxedProcess::GetHelperPath(const std::string& relative_path,
std::string* full_path) {
@@ -78,6 +83,11 @@
return true;
}
+void SandboxedProcess::BindFd(int parent_fd, int child_fd) {
+ ProcessImpl::BindFd(parent_fd, child_fd);
+ bound_fds_.push_back(parent_fd);
+}
+
void SandboxedProcess::DisableSandbox() {
sandboxing_ = false;
}
diff --git a/debugd/src/sandboxed_process.h b/debugd/src/sandboxed_process.h
index bd3f2e6..9341e94 100644
--- a/debugd/src/sandboxed_process.h
+++ b/debugd/src/sandboxed_process.h
@@ -6,6 +6,7 @@
#define DEBUGD_SRC_SANDBOXED_PROCESS_H_
#include <string>
+#include <vector>
#include <brillo/process.h>
@@ -14,7 +15,7 @@
class SandboxedProcess : public brillo::ProcessImpl {
public:
SandboxedProcess();
- ~SandboxedProcess() override = default;
+ ~SandboxedProcess() override;
// Get the full path of a helper executable located at the |relative_path|
// relative to the debugd helpers directory. Return false if the full path
@@ -24,6 +25,8 @@
virtual bool Init();
+ void BindFd(int parent_fd, int child_fd) override;
+
// Disable the default sandboxing for this process.
virtual void DisableSandbox();
@@ -44,6 +47,7 @@
bool access_root_mount_ns_;
std::string user_;
std::string group_;
+ std::vector<int> bound_fds_;
};
} // namespace debugd
diff --git a/debugd/src/systrace_tool.cc b/debugd/src/systrace_tool.cc
index 7046dd6..1d60c68 100644
--- a/debugd/src/systrace_tool.cc
+++ b/debugd/src/systrace_tool.cc
@@ -11,6 +11,7 @@
#include <brillo/process.h>
#include "debugd/src/process_with_output.h"
+#include "debugd/src/sandboxed_process.h"
namespace debugd {
@@ -56,7 +57,7 @@
if (!SandboxedProcess::GetHelperPath(kSystraceHelper, &path))
return;
- ProcessWithOutput p;
+ SandboxedProcess p;
p.SandboxAs(SandboxedProcess::kDefaultUser, kDebugfsGroup);
p.Init();
p.AddArg(path);
