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.

mchlyu · 2026-01-25T05:00:47Z

Hi, I've addressed the original feedback for the PR. Could you please take a look at it?

For the dropping sudo permissions, I set it to only drop if reading a location from the htoprc since that could point to anything versus a run_script inside root's directory. Additionally, a program is only considered trusted if its owned by root. I'm unsure if this is a reasonable enough criteria so I'm open to suggestions to change it.

I've also changed the passing of data to the script to use netstrings instead of TSV.

Valgrind reports that some FDs that are still open, which I believe are the FDs used by fexecve. However, because setting O_CLOEXEC on a file that's a script causes fexecve to fail, I do not know if there is anyway to close these FDs after the exec call.

Gemini helped explain #1844 for what dropping permissions would look like in Linux.

Explorer09 · 2026-01-25T05:20:21Z

RunScript.c

+            return;
+         }
+      }
+   } else if (curr_uid == 0 && (st.st_gid != 0 || st.st_uid != 0)) {


A program installed in /bin or /usr/bin may be owned by root but associated with a non-root group (e.g. "admin" or "wheel"). So I personally consider it too restrictive to require GID to be 0.

That's one issue I can think of. I would review other pieces of code if I have time.

Explorer09 · 2026-01-25T05:22:24Z

RunScript.c

+      return;
+   }
+
+   uid_t curr_uid = getuid();


I think this should be geteuid, not getuid. The difference is not in the sudo use case, but when htop is launched with a SUID privilege.

Explorer09 · 2026-01-25T05:23:32Z

RunScript.c

+   while (pid > 0) {
+      pid /= 10;
+      pid_len++;
+   }


We have countDigits function in XUtils.h. Consider using it.

Apologies for missing that, thanks for pointing it out!

Explorer09 · 2026-01-25T05:50:34Z

For the dropping sudo permissions, I set it to only drop if reading a location from the htoprc since that could point to anything versus a run_script inside root's directory.

We can't assume the run_script will always reside in a directory owned by root, or the script is always owned by root. Check it (see #1866 on how it can be done).

Explorer09 · 2026-01-25T06:05:26Z

RunScript.c

+   int user_len = strlen(this->user);
+   int cmd_len = strlen(Process_getCommand(this));
+   // writes pid_len:PID,user_len:User,cmd_len:Command\n in netstring format
+   xAsprintf(&line, "%d:%d,%d:%s,%d:%s\n", pid_len, row->id, user_len, this->user, cmd_len, Process_getCommand(this));


Why the netstring format? I don't think this makes passing the data safer while it unnecessarily complicates things.

The netstring was meant to address this concern from @BenBE:

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 …

I understand that your original concern is the possibility of cmdline containing \t, and if netstrings don't improve safety, would it be acceptable to sanitize cmdline to remove any \t characters before passing it in the original TSV format?

@mchlyu netstring is safe only if there's an agreement between sender and receiver on the length limit. You notice that the length field can contain any number of digits, so it's possible to craft a netstring that would cause arithmetic overflow on an unguarded receiver.

The security of the format comes with caveats, and I would say you didn't know it yet.

Explorer09 · 2026-01-25T07:21:08Z

RunScript.c

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


I need clarification on what is the use case here.

Why do we have two ways to specify which command to execute?

I do have concern about this implementation: that the presence of one file can prevent the execution of another. It can also bring in an issue when the run_script file can be created or inserted by anyone other than the user running htop when an htop instance is being run.

This was from one of the feature requests in #506

Bonus points for making the script location configurable in /etc/htoprc so that system-wide defaults could be set (having a $XDG_CONFIG_HOME/htop/run_script present would override the global setting still). This would (automatically) also allow to override the script location for a user in their local htoprc (same config reader).

I might've misinterpreted this but I understood it as check for a run_script in $XDG_CONFIG_HOME, and if that doesn't exist, check htoprc.

This is also why I made the assumption earlier that when running htop as sudo, the run_script would need to be in directory owned by root, which could also be an incorrect assumption (my path resolves to /root/.config/htop/run_script when running with sudo).

This was from one of the feature requests in #506

Bonus points for making the script location configurable in /etc/htoprc so that system-wide defaults could be set (having a $XDG_CONFIG_HOME/htop/run_script present would override the global setting still). This would (automatically) also allow to override the script location for a user in their local htoprc (same config reader).

I might've misinterpreted this but I understood it as check for a run_script in $XDG_CONFIG_HOME, and if that doesn't exist, check htoprc.

I would turn down on that part of the feature request because that doesn't help in the security at all.

Having a path in an htoprc file means htop has to memorize it during the settings' load procedure and write the same path when the setting file is saved.

The concrete use case of that extra path is unclear. And nothing can stop the user from making a symlink in the hard-coded location ($XDG_CONFIG_HOME/htop/run_script). So that custom path is redundant and should be turned down.

This is also why I made the assumption earlier that when running htop as sudo, the run_script would need to be in directory owned by root, which could also be an incorrect assumption (my path resolves to /root/.config/htop/run_script when running with sudo).

When htop is run with superuser privilege, the run_script should be run with superuser privilege only if it's owned by the same EUID, and has u+rx permission bits. Otherwise, if SUDO_USER is defined and the run_script is owned by that same user, run with that UID's privilege (setuid(2)). Similar principle applies when htop is run with SUID (i.e. geteuid() != getuid()).

Add ability to run arbitrary commands on keypress

832af1e

Explorer09 reviewed Dec 25, 2025

View reviewed changes

ScriptOutputScreen.c Outdated Show resolved Hide resolved

Explorer09 reviewed Dec 25, 2025

View reviewed changes

RunScript.c Outdated 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

Change script data format to netstrings and add more secure exec method.

408ba08

mchlyu requested a review from BenBE January 25, 2026 04:59

Explorer09 reviewed Jan 25, 2026

View reviewed changes

	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

Uh oh!

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!

mchlyu commented Jan 25, 2026

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 commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Explorer09 Dec 27, 2025 •

edited

Loading

Explorer09 commented Jan 25, 2026 •

edited

Loading