Skip to content

TraceScreen: fix excess copying parent memory page table #1893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GermanAizek wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −54
Open

TraceScreen: fix excess copying parent memory page table #1893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions OpenFilesScreen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ in the source distribution for its full text.
#include <errno.h>
#include <fcntl.h>
#include <inttypes.h>
#include <spawn.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
Expand All @@ -27,6 +28,9 @@ in the source distribution for its full text.
#include "XUtils.h"


// Helper to create a mutable string for argv arrays
#define MUTABLE_STR(s) (char[]){s}

// cf. getIndexForType; must be larger than the maximum value returned.
#define LSOF_DATACOL_COUNT 8

Expand Down Expand Up @@ -106,32 +110,41 @@ static OpenFiles_ProcessData* OpenFilesScreen_getProcessData(pid_t pid) {
return pdata;
}

pid_t child = fork();
if (child == -1) {
pid_t child;
posix_spawnattr_t attr;
posix_spawnattr_init(&attr);
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@Explorer09 Explorer09 Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's set POSIX_SPAWN_RESETIDS to this attr object for security. It drops setuid privileges for us with very little code needed. 🙂

posix_spawn_file_actions_t fa;
posix_spawn_file_actions_init(&fa);
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail, thus return values should be checked.

posix_spawn_file_actions_addclose(&fa, fdpair[0]);
posix_spawn_file_actions_adddup2(&fa, fdpair[1], STDOUT_FILENO);
posix_spawn_file_actions_addclose(&fa, fdpair[1]);
posix_spawn_file_actions_addopen(&fa, STDERR_FILENO, "/dev/null", O_WRONLY | O_NOCTTY, 0);

char buffer[32] = {0};
xSnprintf(buffer, sizeof(buffer), "%d", pid);

char* const args[] = {
MUTABLE_STR("lsof"),
MUTABLE_STR("-P"),
MUTABLE_STR("-o"),
MUTABLE_STR("-p"),
buffer,
MUTABLE_STR("-F"),
NULL
};

int spawn_ret = posix_spawnp(&child, "lsof", &fa, &attr, args, NULL);
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

@Explorer09 Explorer09 Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Comment on lines +138 to +139
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource cleanup should be reverse of acquisition.


if (spawn_ret != 0) {
close(fdpair[1]);
close(fdpair[0]);
pdata->error = 1;
return pdata;
}

if (child == 0) {
close(fdpair[0]);
dup2(fdpair[1], STDOUT_FILENO);
close(fdpair[1]);
int fdnull = open("/dev/null", O_WRONLY);
if (fdnull < 0) {
exit(1);
}

dup2(fdnull, STDERR_FILENO);
close(fdnull);
char buffer[32] = {0};
xSnprintf(buffer, sizeof(buffer), "%d", pid);
// Use of NULL in variadic functions must have a pointer cast.
// The NULL constant is not required by standard to have a pointer type.
execlp("lsof", "lsof", "-P", "-o", "-p", buffer, "-F", (char*)NULL);
exit(127);
}
close(fdpair[1]);

OpenFiles_Data* item = &(pdata->data);
Expand Down
70 changes: 36 additions & 34 deletions TraceScreen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ in the source distribution for its full text.
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <spawn.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
Expand All @@ -28,6 +29,9 @@ in the source distribution for its full text.
#include "XUtils.h"


// Helper to create a mutable string for argv arrays
#define MUTABLE_STR(s) (char[]){s}

static const char* const TraceScreenFunctions[] = {"Search ", "Filter ", "AutoScroll ", "Stop Tracing ", "Done ", NULL};

static const char* const TraceScreenKeys[] = {"F3", "F4", "F8", "F9", "Esc"};
Expand Down Expand Up @@ -78,45 +82,43 @@ bool TraceScreen_forkTracer(TraceScreen* this) {
if (fcntl(fdpair[1], F_SETFL, O_NONBLOCK) < 0)
goto err;

pid_t child = fork();
if (child == -1)
pid_t child;
posix_spawnattr_t attr;
posix_spawnattr_init(&attr);
posix_spawn_file_actions_t fa;
posix_spawn_file_actions_init(&fa);
posix_spawn_file_actions_addclose(&fa, fdpair[0]);
posix_spawn_file_actions_adddup2(&fa, fdpair[1], STDOUT_FILENO);
posix_spawn_file_actions_adddup2(&fa, fdpair[1], STDERR_FILENO);
posix_spawn_file_actions_addclose(&fa, fdpair[1]);

char buffer[32] = {0};
xSnprintf(buffer, sizeof(buffer), "%d", Process_getPid(this->super.process));

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
Comment on lines +98 to +105
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's become borderline unreadable. Much more noise compared to previously.

Copy link
Copy Markdown
Contributor

@Explorer09 Explorer09 Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@Explorer09 Explorer09 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I've made the public version of the code review request as #1915. And I would invite @GermanAizek to take a look of the feature and code changes. It would be more secure than using posix_spawn() as proposed here.


int spawn_ret = args ? posix_spawnp(&child, args[0], &fa, &attr, args, NULL) : -1;

posix_spawnattr_destroy(&attr);
posix_spawn_file_actions_destroy(&fa);

if (spawn_ret != 0) {
goto err;

if (child == 0) {
close(fdpair[0]);

dup2(fdpair[1], STDOUT_FILENO);
dup2(fdpair[1], STDERR_FILENO);
close(fdpair[1]);

char buffer[32] = {0};
xSnprintf(buffer, sizeof(buffer), "%d", Process_getPid(this->super.process));

#if defined(HTOP_FREEBSD) || defined(HTOP_OPENBSD) || defined(HTOP_NETBSD) || defined(HTOP_DRAGONFLYBSD) || defined(HTOP_SOLARIS)
// Use of NULL in variadic functions must have a pointer cast.
// The NULL constant is not required by standard to have a pointer type.
execlp("truss", "truss", "-s", "512", "-p", buffer, (void*)NULL);

// Should never reach here, unless execlp fails ...
const char* message = "Could not execute 'truss'. Please make sure it is available in your $PATH.";
(void)! write(STDERR_FILENO, message, strlen(message));
#elif defined(HTOP_LINUX)
execlp("strace", "strace", "-T", "-tt", "-s", "512", "-p", buffer, (void*)NULL);

// Should never reach here, unless execlp fails ...
const char* message = "Could not execute 'strace'. Please make sure it is available in your $PATH.";
(void)! write(STDERR_FILENO, message, strlen(message));
#else // HTOP_DARWIN, HTOP_PCP == HTOP_UNSUPPORTED
const char* message = "Tracing unavailable on not supported system.";
(void)! write(STDERR_FILENO, message, strlen(message));
#endif
Comment on lines -109 to -112
Copy link
Copy Markdown
Member

@BenBE BenBE Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behaviour change on platforms not supporting truss/strace …


exit(127);
}

FILE* fp = fdopen(fdpair[0], "r");
if (!fp)
if (!fp) {
kill(child, SIGTERM);
waitpid(child, NULL, 0);
goto err;
}

close(fdpair[1]);

Expand Down
Loading