Repository navigation
Avoid bare uses of procfs and implement proc_file_utils for some platforms. - #3460
3405691582 wants to merge 5 commits into
Conversation
| } | ||
|
|
||
| Result<std::string> GetExecutablePath(const pid_t pid) { | ||
| #if defined(OpenBSD) && OpenBSD >= 202610 |
There was a problem hiding this comment.
Is there an expected scenario where defined(OpenBSD) is false in a file that is only built in bazel's @platforms//os:openbsd mode? Maybe we should just take the compile error when that happens.
There was a problem hiding this comment.
No, since <sys/param.h> always includes it. But even if it's undefined, this should be treated as 0, so it wouldn't error in that case anyway.
| } | ||
|
|
||
| Result<std::vector<std::string>> GetCmdArgs(const pid_t pid) { | ||
| CF_EXPECT_EQ(CF_EXPECT(OwnerUid(pid)), getuid()); |
There was a problem hiding this comment.
Nested CF_EXPECT / CF_EXPECT_<cmp> calls produce excessively verbose output on errors, as each call will preprocessor-stringify its arguments under validation to report in the message. The arguments are already preprocessor-expanded at this point so the error report includes all the CF_EXPECT machinery in the message.
| Result<std::vector<pid_t>> CollectPidsByExecPath(const std::string& exec_path, | ||
| const uid_t uid) { | ||
| std::vector<pid_t> output_pids; | ||
| for (const pid_t pid : CF_EXPECT(CollectPids(uid))) { | ||
| Result<std::string> pid_exec_path = GetExecutablePath(pid); | ||
| if (pid_exec_path.has_value() && *pid_exec_path == exec_path) { | ||
| output_pids.push_back(pid); | ||
| } | ||
| } | ||
| return output_pids; | ||
| } | ||
|
|
||
| Result<std::vector<pid_t>> CollectPidsByArgv0(const std::string& expected_argv0, | ||
| const uid_t uid) { | ||
| std::vector<pid_t> output_pids; | ||
| for (const pid_t pid : CF_EXPECT(CollectPids(uid))) { | ||
| Result<std::vector<std::string>> argv = GetCmdArgs(pid); | ||
| if (argv.has_value() && !argv->empty() && argv->front() == expected_argv0) { | ||
| output_pids.push_back(pid); | ||
| } | ||
| } | ||
| return output_pids; | ||
| } |
There was a problem hiding this comment.
CollectPidsByExecPath and CollectPidsByArgv0 are identical between proc_file_utils_darwin.cc and proc_file_utils_openbsd.cc, and effectively the same as the implementations in the existing proc_file_utils.cc, now used for Linux.
Is it possible to deduplicate these between the different platforms? If you want to minimize the blast radius of the change, the existing build target could keep the same header and have a common proc_file_utils.cc file, with a select for a platform-specific additional file. Most of the current proc_file_utils.cc can be proc_file_utils_linux.cc.
There was a problem hiding this comment.
Yes, I've moved the Linux-specific methods to proc_file_utils_linux and kept CollectPidsByExecPath and CollectPidsByArgv0 in proc_file_utils.cc for use on all platforms.
| "sysctl(kern.proc.pid.{}) failed: {}", pid, StrError(errno)); | ||
| CF_EXPECTF(size == sizeof(kp), "No process with pid {}", pid); | ||
| return kp; | ||
| } |
There was a problem hiding this comment.
Similar to the comment below on CollectPidsByExecPath, this is identical between darwin and openbsd. It would be nice to have as much overlap as possible between different platforms. Maybe shared darwin/openbsd implementations could be consolidated under a proc_file_utils_bsd.cc with a private header in the same bazel target?
There was a problem hiding this comment.
These aren't completely identical, since the mibs are different. I avoided adding too much structure even though there's overlap between the implementations, they're not strictly identical, and sometimes it's slightly clearer to be a little duplicative rather than adding artifice too deeply.
If you feel strongly about it, we could do something, perhaps...
There was a problem hiding this comment.
I see your point, even though the call hierarchy is similar each implementation is just different enough at every level that it would be pretty awkward to extract the common pieces.
| sizeof(struct kinfo_proc), | ||
| 0}; |
There was a problem hiding this comment.
There's different arguments required to be supplied in the mib. (Think of these like ioctls or syscalls; there may be some shared names, but the arguments/flags may be different.)
There was a problem hiding this comment.
My issue was that on my first read I missed this quote
The fifth level name is the size of the
struct kinfo_procand the sixth level name is the number of structures to return.
Currently, proc_file_utils.cc implements the entire interface against procfs, even though CollectPidsByExecPath and CollectPidsByArgv0 are built purely on top of CollectPids, GetExecutablePath, and GetCmdArgs. Ahead of adding non-Linux implementations, we move the procfs code into proc_file_utils_linux.cc and keep those platform-neutral helpers in proc_file_utils.cc so that every platform can share them. For now, there should be no functional changes.
Only Linux has procfs, so every other platform needs its own implementation of this interface. Most of them use sysctl(2), but differ in the arguments they take and in the layout of the data they return. GetExecutablePath() needs extra work, since nothing standard answers it. OpenBSD 8.0 added getexecpath(3), which is used when built for that release or later, but it only covers the calling process; for other processes, and on earlier releases, argv[0] is resolved the way execvp(3) would have, as an approximation.
macOS has no procfs either, and its sysctl(2) process interface takes different arguments and returns a different kinfo_proc layout from OpenBSD's; argv and the environment come from KERN_PROCARGS2 in one buffer. GetExecutablePath() uses proc_pidpath(3), which is exact for any process. This has only been compiled, against the macOS SDK; it has not been run.
GetPidsUsingFiles() uses procfs to refuse to delete a prior instance directory that a running process still has open. On platforms without procfs the check silently passes. Fixing this properly can be involved, since it requires translating pids to fds and fds to paths, which is not always straightforward. Outside Linux, use the same behaviour as the sandbox path.
RestartRunCvd() re-executes itself through /proc/self/exe. procfs is not available outside Linux, so elsewhere the execv fails. GetExecutablePath() answers the same question on every platform. On Linux this now execs the path the link points at rather than the link itself, so a run_cvd whose binary was replaced on disk restarts into the new binary instead of the deleted inode.
Here, we port proc_file_utils to other platforms that don't have procfs. Subsequently, we address some uses of procfs that either are better served by the proc_file_utils that are now multiplatform or guard the behavior to be procfs-specific. More details in individual commit messages.
Note that the macOS implementation has only been built for the target, not run.