TraceScreen: fix excess copying parent memory page table#1893
TraceScreen: fix excess copying parent memory page table#1893GermanAizek wants to merge 1 commit intohtop-dev:mainfrom
Conversation
GermanAizek
force-pushed
the
migrate-to-posixspawn
branch
2 times, most recently
from
48089bd to
35c7070
Compare
posix_spawn() is a more lightweight and modern alternative to fork()/exec(), which are used inside popen(). The main advantage of posix_spawn is that it can create a new process without completely duplicating the address space of the parent process. References: - https://blog.famzah.net/2018/12/19/posix_spawn-performance-benchmarks-and-usage-examples/ - https://lobste.rs/s/smbsd5/fork_road - https://www.reddit.com/r/C_Programming/comments/1lvdhp2/fork_vs_posix_spawn/
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Does not return a readable error message to the user on failure.
| if (child == -1) { | ||
| pid_t child; | ||
| posix_spawnattr_t attr; | ||
| posix_spawnattr_init(&attr); |
There was a problem hiding this comment.
This can fail, thus return values should be checked.
Also, there are no attributes set on this empty object, thus for posix_spawnp the attributes could be passed as NULL.
There was a problem hiding this comment.
Let's set POSIX_SPAWN_RESETIDS to this attr object for security. It drops setuid privileges for us with very little code needed. 🙂
| posix_spawnattr_t attr; | ||
| posix_spawnattr_init(&attr); | ||
| posix_spawn_file_actions_t fa; | ||
| posix_spawn_file_actions_init(&fa); |
There was a problem hiding this comment.
This can fail, thus return values should be checked.
| NULL | ||
| }; | ||
|
|
||
| int spawn_ret = posix_spawnp(&child, "lsof", &fa, &attr, args, NULL); |
There was a problem hiding this comment.
The environment envp should not be passed as NULL. This is accepted by some implementations, but is not generally portable.
Passing an (mostly¹) empty environment instead should suffice (also makes program output more predictable).
¹override LC_ALL=C.UTF-8 and pass on PATH (optional).
There was a problem hiding this comment.
Declare extern char** environ; at the top of the .c file and pass environ as envp instead of NULL.
| posix_spawnattr_destroy(&attr); | ||
| posix_spawn_file_actions_destroy(&fa); |
There was a problem hiding this comment.
Resource cleanup should be reverse of acquisition.
| #else // HTOP_DARWIN, HTOP_PCP == HTOP_UNSUPPORTED | ||
| const char* message = "Tracing unavailable on not supported system."; | ||
| (void)! write(STDERR_FILENO, message, strlen(message)); | ||
| #endif |
There was a problem hiding this comment.
Behaviour change on platforms not supporting truss/strace …
| char* const* args; | ||
| #if defined(HTOP_FREEBSD) || defined(HTOP_OPENBSD) || defined(HTOP_NETBSD) || defined(HTOP_DRAGONFLYBSD) || defined(HTOP_SOLARIS) | ||
| args = (char* const[]){MUTABLE_STR("truss"), MUTABLE_STR("-s"), MUTABLE_STR("512"), MUTABLE_STR("-p"), buffer, NULL}; | ||
| #elif defined(HTOP_LINUX) | ||
| args = (char* const[]){MUTABLE_STR("strace"), MUTABLE_STR("-T"), MUTABLE_STR("-tt"), MUTABLE_STR("-s"), MUTABLE_STR("512"), MUTABLE_STR("-p"), buffer, NULL}; | ||
| #else | ||
| args = NULL; | ||
| #endif |
There was a problem hiding this comment.
That's become borderline unreadable. Much more noise compared to previously.
There was a problem hiding this comment.
@GermanAizek I have a concern about MUTABLE_STR macro. It might not do what you want with the syntax, and I feel it strange when it doesn't trigger a "-Wcast-qual" warning from your compiler. (I have a better workaround for this, but it is part of a private email to BenBE that's I'm not sure I can share it publicly for now.)
@BenBE Please check your mailbox. I have sent a private email requesting code review about a security feature when launching exteral programs. It (and issue #1844) would be related to this PR.
|
Overall I'm not sure if this API change is worth it. Modern OS kernels should have implemented the copy-on-write mechanisms for process pages making the performance differences between @GermanAizek , Do you have benchmark data to back your claim about using The Linux man page about posix_spawn doesn't say about performance being a primary reason of using Also: I reported issue #1844, about the security of launching external programs. And this PR code is still vulnerable (to, for example, PATH environment variable attack). EDIT just to mention two more things: (1) For security reasons we need |
3 participants
posix_spawn() is a more lightweight and modern alternative to fork()/exec(), which are used inside popen(). The main advantage of posix_spawn is that it can create a new process without completely duplicating the address space of the parent process.
References: