Add ability to run arbitrary commands on keypress #1843

mchlyu · 2025-12-25T04:38:03Z

Addresses #506

All input to script is given on stdin as a sequence of "PID" "USER" "COMMAND" in TSV format.
Users can configure htoprc to provide a script path setting that will act as fallback if no file exists at $XDG_CONFIG_HOME/htop/run_script.

I tested functionality using this gdb script on Ubuntu.

#!/bin/bash

# read input from stdin, expecting TSV formatted tuple [int, string, string]
while IFS=$'\t' read -r pid usr cmd; do
    gdb -p "$pid" -batch -ex "thread apply all bt" -ex "quit"
done

Created new InfoScreen pager heavily inspired by OpenFilesScreen and TraceScreen to display stdout and stderr from script.
Also updated help screen and man page to include this feature.

Valgrind did not report any new memory leaks.

I'm open to any suggestions for improving the structure, security, etc. of the code or clarity of the documentation.
This is my first contribution to this project (and open source in general), so please let me know if I’ve missed any conventions.

ScriptOutputScreen.c

Explorer09 · 2025-12-25T09:04:20Z

There are a lot of feature suggestions of running arbitrary commands through htop, but almost all the pull request code suffers from a problem:

htop is often run with elevated privileges (e.g. sudo), and there is a need for (1) an indicator on whether the external program should be run with elevated privileges or dropped privileges, and (2) a security check that, if the script is meant to be run with elevated privileges, it's not installed by users other than root (which can prevent accidental running of malicious programs).

So this pull request doesn't have either security mechanism yet.

RunScript.c

Explorer09 · 2025-12-25T09:12:45Z

RunScript.c

+      const char* path = String_cat(home, "/htop/run_script");
+      FILE* file = fopen(path, "r");
+      if (file) {
+         execl(path, path, NULL);


Suggested change

execl(path, path, NULL);

execl(path, path, (void*)NULL);

One more thing. Since we need to verify the ownership of the run_script to be executed, the execl, execv family of API would not fit the job.

Consider the fexecve(3) or execveat(2) API instead.

Got it, thank you for the clarification and suggestions. I'll see if I'm able to implement something that fits #1844 to address this security issue.

Explorer09 · 2025-12-25T09:13:30Z

RunScript.c

+      } else {
+         // check if htoprc has something
+         const char* htoprc_path = st->host->settings->scriptLocation;
+         execl(htoprc_path, htoprc_path, NULL);


Suggested change

execl(htoprc_path, htoprc_path, NULL);

execl(htoprc_path, htoprc_path, (void*)NULL);

BenBE

Were parts of this PR assisted or written aided by LLMs?

Also, as the command line of a process may contain anything, A more robust solution would provide the exact length of the data to follow and push the data after that literal. Arguments are split by \0 by the kernel, but since programs can overwrite that buffer anything goes …

Also, given that there are some other (similar) screens, what do you think of merging these (in a separate) PR to reduce code duplication?

BenBE · 2025-12-25T13:02:11Z

RunScript.c

+   char pid[pid_length + 1];
+   snprintf(pid, pid_length + 1, "%d", row->id);


No VLA. Also use internal xSnprintf APIs. Also, why not allocate a static buffer that's large enough for any %d like 32 bytes? Or use multiple writes for the target FD?

Ah I missed xSnprintf when I was going through xUtils, apologies.
I chose to do all these string concatenations and a singular write because I thought (although I have no data to back this up) a write is much more expensive than string operations, but I see how it makes the code messier. I'm open to switching to multiple writes though.

@mchlyu There is also xAsprintf if you need to allocate and print the format string in one call.

Ooh that might be useful, thanks for pointing that out!

BenBE · 2025-12-25T13:04:04Z

RunScript.c

+   char* pid_str = String_cat(pid, "\t");
+   char* user = String_cat(this->user, "\t");
+   char* cmd = String_cat(Process_getCommand(this), "\n");
+   char* user_and_cmd = String_cat(user, cmd);
+   char* line = String_cat(pid_str, user_and_cmd);


Why not make this multiple writes into stdin of the new process instead? Apart from xSnprintf being the better tool for handling the %d\t%s\t part for PID+username here anyway.

What I said here, although I could be wrong or the performance difference doesn't matter that much or both.

The difference performance-wise is one vs. multiple syscalls, but given that malloc might syscall itself (mmap), there is not much of a difference in practice. Also I don't consider this a hot path, thus there's no pressing need for optimizations there.

Okay, thanks for clarifying; multiple writes will probably be much cleaner here.

I wonder if we can use a separator other than tab.

The problem is that the cmdline can contain almost any character, including tab. I know it could be a pain to securely parse a cmdline string when it contains special characters, but we probably have to pass the cmdline string securely to the run script.

RunScript.c

Settings.c

ScriptOutputScreen.c

BenBE · 2025-12-25T13:10:51Z

ScriptOutputScreen.c

+         break;
+      }
+
+      if (res > 0) {


Invert condition and continue to reduce level of indentation. Cf. XReadFile API for other places that need this.

Got it, removed one level of indentation so far.
Regarding xReadFile, is that meant to be a function in xUtils? Because I can't find it in that file nor does searching online yield anything so I'm a bit confused.

Has been moved in the latest sources to Compat.c as readfd_internal with Compat_readfile and Compat_readfileat being official callers.

Got it, thanks for pointing out the new location.

RunScript.c

mchlyu · 2025-12-27T00:44:28Z

@BenBE
I used Gemini to help me understand the codebase, explain portions of the feature request, help debug some segfaults, and suggest approaches. No code was copied from Gemini.
The last line of this PR and the gdb script were written by ChatGPT.
Should I update the PR to say co-authored by Gemini or GPT?

Thanks for pointing out a more robust approach. Regarding the command line of the process, could you please elaborate on how would I get the exact length of the command line, since right now I take whatever Process_getCommand gives me, and wherever that's terminated is the string I'd copy. If any program overwrote that string, I wouldn't know it's been changed in the first place right?

For merging the screens, that sounds like a good idea to me, although I'm less clear on which screen(s) this one is similar enough to be combined with. Perhaps something like CommandScreen?

BenBE · 2025-12-31T21:29:30Z

For merging the existing screens, the most likely candidates are the strace screen, which IIRC is already based on the Command Screen implementation. Thus for running your own commands a similar base can be used.

Add ability to run arbitrary commands on keypress

832af1e

Explorer09 reviewed Dec 25, 2025

View reviewed changes

ScriptOutputScreen.c Show resolved Hide resolved

Explorer09 reviewed Dec 25, 2025

View reviewed changes

RunScript.c Show resolved Hide resolved

Explorer09 reviewed Dec 25, 2025

View reviewed changes

BenBE requested changes Dec 25, 2025

View reviewed changes

Explorer09 mentioned this pull request Dec 25, 2025

Security check of external programs before launching #1844

Open

	execl(htoprc_path, htoprc_path, NULL);
	execl(htoprc_path, htoprc_path, (void*)NULL);

		char pid[pid_length + 1];
		snprintf(pid, pid_length + 1, "%d", row->id);

Uh oh!

Add ability to run arbitrary commands on keypress #1843

Are you sure you want to change the base?

Add ability to run arbitrary commands on keypress #1843

Conversation

mchlyu commented Dec 25, 2025

Uh oh!

Uh oh!

Explorer09 commented Dec 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenBE left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mchlyu commented Dec 27, 2025

Uh oh!

BenBE commented Dec 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Explorer09 Dec 27, 2025 •

edited

Loading