-
-
Notifications
You must be signed in to change notification settings - Fork 599
Add ability to run arbitrary commands on keypress #1843
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||
| /* | ||||||
| htop - RunScript.h | ||||||
| (C) 2025 htop dev team | ||||||
| Released under the GNU GPLv2+, see the COPYING file | ||||||
| in the source distribution for its full text. | ||||||
| */ | ||||||
|
|
||||||
| #include "config.h" // IWYU pragma: keep | ||||||
|
|
||||||
| #include "RunScript.h" | ||||||
|
|
||||||
| #include <assert.h> | ||||||
| #include <errno.h> | ||||||
| #include <fcntl.h> | ||||||
| #include <stdbool.h> | ||||||
| #include <stdio.h> | ||||||
| #include <stdlib.h> | ||||||
| #include <string.h> | ||||||
| #include <unistd.h> | ||||||
|
|
||||||
| #include "Action.h" | ||||||
| #include "InfoScreen.h" | ||||||
| #include "MainPanel.h" | ||||||
| #include "Object.h" | ||||||
| #include "Panel.h" | ||||||
| #include "Process.h" | ||||||
| #include "Row.h" | ||||||
| #include "ScriptOutputScreen.h" | ||||||
| #include "Settings.h" | ||||||
| #include "XUtils.h" | ||||||
|
|
||||||
|
|
||||||
| static void write_row(Row* row, int write_fd) { | ||||||
| Process* this = (Process*) row; | ||||||
| assert(Object_isA((const Object*) this, (const ObjectClass*) &Process_class)); | ||||||
|
|
||||||
| int pid_length = snprintf(NULL, 0, "%d", row->id); | ||||||
| char pid[pid_length + 1]; | ||||||
| snprintf(pid, pid_length + 1, "%d", row->id); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No VLA. Also use internal
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I missed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mchlyu There is also
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh that might be useful, thanks for pointing that out! |
||||||
|
|
||||||
| 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); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not make this multiple writes into
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I said here, although I could be wrong or the performance difference doesn't matter that much or both.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks for clarifying; multiple writes will probably be much cleaner here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can use a separator other than tab. The problem is that the |
||||||
|
|
||||||
| // writes PID\tUser\tCommand\n, using TSV format | ||||||
| size_t count = strlen(line); | ||||||
| char* line_start = line; | ||||||
| while (count > 0) { | ||||||
| ssize_t res = write(write_fd, line_start, strlen(line)); | ||||||
| if ((res == -1 && errno != EINTR) || res == 0) | ||||||
| break; | ||||||
|
|
||||||
| count -= res; | ||||||
| line_start += res; | ||||||
| } | ||||||
|
|
||||||
| free(pid_str); | ||||||
| free(user); | ||||||
| free(cmd); | ||||||
| free(user_and_cmd); | ||||||
| free(line); | ||||||
| } | ||||||
|
|
||||||
| void RunScript(State* st) { | ||||||
| int child_read[2] = {0, 0}; | ||||||
| int child_write[2] = {0, 0}; | ||||||
|
|
||||||
| if (pipe(child_read) == -1) | ||||||
| return; | ||||||
| if (pipe(child_write) == -1) { | ||||||
| close(child_read[0]); | ||||||
| close(child_read[1]); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| pid_t child = fork(); | ||||||
| if (child == -1) { | ||||||
| close(child_read[0]); | ||||||
| close(child_read[1]); | ||||||
| close(child_write[0]); | ||||||
| close(child_write[1]); | ||||||
| fprintf(stderr, "fork failed\n"); | ||||||
| return; | ||||||
| } else if (child == 0) { | ||||||
| close(child_read[1]); | ||||||
| dup2(child_read[0], STDIN_FILENO); | ||||||
| close(child_read[0]); | ||||||
|
|
||||||
| close(child_write[0]); | ||||||
| dup2(child_write[1], STDOUT_FILENO); | ||||||
| dup2(child_write[1], STDERR_FILENO); | ||||||
| close(child_write[1]); | ||||||
|
|
||||||
| char* home = getenv("XDG_CONFIG_HOME"); | ||||||
| if (!home) | ||||||
| home = String_cat(getenv("HOME"), "./config"); | ||||||
|
|
||||||
| const char* path = String_cat(home, "/htop/run_script"); | ||||||
| FILE* file = fopen(path, "r"); | ||||||
| if (file) { | ||||||
| execl(path, path, NULL); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more thing. Since we need to verify the ownership of the Consider the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| // should not reach here unless execl fails | ||||||
| fprintf(stderr, "error excuting %s\n", path); | ||||||
| perror("execl"); | ||||||
| } else { | ||||||
| // check if htoprc has something | ||||||
| const char* htoprc_path = st->host->settings->scriptLocation; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was from one of the feature requests in #506
I might've misinterpreted this but I understood it as check for a This is also why I made the assumption earlier that when running htop as sudo, the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 (
When htop is run with superuser privilege, the |
||||||
| execl(htoprc_path, htoprc_path, NULL); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| // only reach here if execl fails | ||||||
| fprintf(stderr, "error executing %s from htoprc", htoprc_path); | ||||||
| fprintf(stderr, "if you expected your runscript to be executed, htop looked for it at %s", path); | ||||||
| perror("execl"); | ||||||
| } | ||||||
| exit(1); | ||||||
|
mchlyu marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
|
|
||||||
| close(child_read[0]); | ||||||
| close(child_write[1]); | ||||||
|
|
||||||
| bool anyTagged = false; | ||||||
| Panel* super = &st->mainPanel->super; | ||||||
| for (int i = 0; i < Panel_size(super); i++) { | ||||||
| Row* row = (Row*) Panel_get(super, i); | ||||||
| if (row->tag) { | ||||||
| write_row(row, child_read[1]); | ||||||
| anyTagged = true; | ||||||
| } | ||||||
| } | ||||||
| // if nothing was tagged, operate on the highlighted row | ||||||
| if (!anyTagged) { | ||||||
| Row* row = (Row*) Panel_getSelected(super); | ||||||
| if (row) | ||||||
| write_row(row, child_read[1]); | ||||||
| } | ||||||
|
|
||||||
| // tell script/child we're done with sending input | ||||||
| close(child_read[1]); | ||||||
|
|
||||||
| const Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel); | ||||||
| if (!p) | ||||||
| return; | ||||||
|
|
||||||
| assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class)); | ||||||
| ScriptOutputScreen* sos = ScriptOutputScreen_new(p); | ||||||
| if (fcntl(child_write[0], F_SETFL, O_NONBLOCK) >= 0) { | ||||||
| ScriptOutputScreen_SetFd(sos, child_write[0]); | ||||||
| InfoScreen_run((InfoScreen*)sos); | ||||||
| } | ||||||
| ScriptOutputScreen_delete((Object*)sos); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #ifndef RUNSCRIPT_Process | ||
| #define RUNSCRIPT_Process | ||
| /* | ||
| htop - RunScript.h | ||
| (C) 2025 htop dev team | ||
| Released under the GNU GPLv2+, see the COPYING file | ||
| in the source distribution for its full text. | ||
| */ | ||
|
|
||
| #include "Action.h" | ||
|
|
||
|
|
||
| typedef struct Node_ { | ||
| char* line; | ||
| struct Node_* next; | ||
| } Node; | ||
|
|
||
|
|
||
| void RunScript(State*); | ||
|
|
||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| /* | ||
| htop - ScriptOutputScreen.c | ||
| (C) 2025 htop dev team | ||
| Released under the GNU GPLv2+, see the COPYING file | ||
| in the source distribution for its full text. | ||
| */ | ||
|
|
||
| #include "config.h" // IWYU pragma: keep | ||
|
|
||
| #include "ScriptOutputScreen.h" | ||
|
|
||
| #include <assert.h> | ||
| #include <errno.h> | ||
| #include <stdlib.h> | ||
| #include <unistd.h> | ||
|
|
||
| #include "Panel.h" | ||
| #include "ProvideCurses.h" | ||
| #include "XUtils.h" | ||
|
|
||
|
|
||
| ScriptOutputScreen* ScriptOutputScreen_new(const Process* process) { | ||
| ScriptOutputScreen* this = xCalloc(1, sizeof(ScriptOutputScreen)); | ||
| Object_setClass(this, Class(ScriptOutputScreen)); | ||
| // this fd needs to be set later | ||
| this->read_fd = -1; | ||
| this->data_head = NULL; | ||
| this->data_tail = &this->data_head; | ||
| return (ScriptOutputScreen*) InfoScreen_init(&this->super, process, NULL, LINES - 2, " "); | ||
| } | ||
|
|
||
| void ScriptOutputScreen_SetFd(ScriptOutputScreen* this, int fd) { | ||
| this->read_fd = fd; | ||
| } | ||
|
|
||
| void ScriptOutputScreen_delete(Object* this) { | ||
| // free the linked list and close fd | ||
| assert(Object_isA((const Object*) this, (const ObjectClass*) &ScriptOutputScreen_class)); | ||
| Node* walk = ((ScriptOutputScreen*)this)->data_head; | ||
| while (walk) { | ||
| free(walk->line); | ||
| Node* next = walk->next; | ||
| free(walk); | ||
| walk = next; | ||
| } | ||
| close(((ScriptOutputScreen*)this)->read_fd); | ||
| free(InfoScreen_done((InfoScreen*)this)); | ||
| } | ||
|
|
||
| static void ScriptOutputScreen_scan(InfoScreen* super) { | ||
| Panel* panel = super->display; | ||
| int idx = Panel_getSelectedIndex(panel); | ||
| Panel_prune(panel); | ||
|
|
||
| char buffer[8192]; | ||
| ScriptOutputScreen* sos = ((ScriptOutputScreen*)super); | ||
| assert(Object_isA((const Object*) sos, (const ObjectClass*) &ScriptOutputScreen_class)); | ||
|
|
||
| // redraw existing stuff in the screen first | ||
| Node* walk = sos->data_head; | ||
| while (walk) { | ||
| InfoScreen_addLine(super, walk->line); | ||
| walk = walk->next; | ||
| } | ||
|
|
||
| for (;;) { | ||
| ssize_t res = read(sos->read_fd, buffer, sizeof(buffer) - 1); | ||
| if (res < 0) { | ||
| if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK) | ||
| continue; | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| if (res == 0) { | ||
| break; | ||
| } | ||
|
|
||
| if (res > 0) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Invert condition and
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, removed one level of indentation so far.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Has been moved in the latest sources to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks for pointing out the new location. |
||
| int start = 0; | ||
| int num_tabs = 0; | ||
|
mchlyu marked this conversation as resolved.
Outdated
|
||
| for (int i = 0; i <= res; i++) { | ||
| num_tabs += (buffer[i] == '\t'); | ||
| // split line when find \n or exhaust buffer | ||
| if (i == res || buffer[i] == '\n') { | ||
| buffer[i] = '\0'; | ||
| char* str; | ||
| if (num_tabs > 0) { | ||
| // manually replace all \t with TABSIZE spaces | ||
| str = xMalloc((num_tabs * TABSIZE + i - start + 1) * sizeof(char)); | ||
| int index = 0; | ||
| for (int j = start; j <= i; j++) { | ||
|
mchlyu marked this conversation as resolved.
Outdated
|
||
| if (buffer[j] == '\t') { | ||
| for (int k = 0; k < TABSIZE; k++) { | ||
| str[index++] = ' '; | ||
| } | ||
| } else { | ||
| str[index++] = buffer[j]; | ||
| } | ||
| } | ||
| } else { | ||
| str = buffer + start; | ||
| } | ||
| InfoScreen_addLine(super, str); | ||
| // store line for next redraw | ||
| *(sos->data_tail) = xMalloc(sizeof(Node)); | ||
| (*sos->data_tail)->line = xStrdup(str); | ||
| (*sos->data_tail)->next = NULL; | ||
| *(&sos->data_tail) = &((*sos->data_tail)->next); | ||
|
|
||
| if (num_tabs > 0) | ||
| free(str); | ||
| start = i + 1; | ||
| num_tabs = 0; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Panel_setSelected(panel, idx); | ||
| } | ||
|
|
||
| static void ScriptOutputScreen_draw(InfoScreen* this ) { | ||
| InfoScreen_drawTitled(this, "Output of script for process %d - %s", Process_getPid(this->process), Process_getCommand(this->process)); | ||
| } | ||
|
|
||
| const InfoScreenClass ScriptOutputScreen_class = { | ||
| .super = { | ||
| .extends = Class(Object), | ||
| .delete = ScriptOutputScreen_delete | ||
| }, | ||
| .scan = ScriptOutputScreen_scan, | ||
| .draw = ScriptOutputScreen_draw | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.