Skip to content

Action: verify the selected row is a Process before casting #2151

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
pwilke wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Action: verify the selected row is a Process before casting #2151

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f15a39
Action: verify the selected row is a Process before casting
pwilke Nov 5, 2026
e87dc28
Action: re-sync the active table when Setup closes
pwilke Nov 8, 2026
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
39 changes: 23 additions & 16 deletions Action.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,10 @@ Object* Action_pickFromVector(State* st, Panel* list, int x, bool follow) {

// ----------------------------------------

static inline bool setActiveScreen(Settings* settings, State* st, unsigned int ssIdx);

static void Action_runSetup(State* st) {
const Settings* settings = st->host->settings;
Settings* settings = st->host->settings;
ScreenManager* scr = ScreenManager_new(st->header, st->host, st, true);
CategoriesPanel_new(scr, st->header, st->host);
ScreenManager_run(scr, NULL, NULL, "Setup");
Expand All @@ -109,6 +111,11 @@ static void Action_runSetup(State* st) {
CRT_setMouse(settings->enableMouse);
Header_writeBackToSettings(st->header);
}

/* Setup may have repointed settings->ss (e.g. reordering the Screens list)
without updating host->activeTable; re-sync them so later actions act on
the correct table. */
setActiveScreen(settings, st, settings->ssIndex);

@coderabbitai coderabbitai Bot Nov 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Functional Correctness · Low value

Supported by static analysis

Script executed:

rg -n -C 3 'following|activeTable|\.id\s*=|->id\s*=' --glob '*.[ch]' | head -n 240

Repository: htop-dev/htop

Length of output: 12061

Script executed:

git diff 50919007b00ef657acdcea084c31a22b152c3799 e87dc28d5e7bebdb3adb6c582b5b745bf25fc11e -- Action.c
rg -n -F -e 'setActiveScreen' -e 'actionSetup' -e 'HTOP_KEEP_FOLLOWING' -- Action.c MainPanel.c ScreenManager.c
rg -n -F -e 'following' -e 'Instance_setId' -e 'Process_setPid' -- Table.c Action.c MainPanel.c Process.h Process.c pcp/
nl -ba Action.c | sed -n '1,160p'

Repository: htop-dev/htop

Length of output: 14407

Script executed:

nl -ba Action.c | sed -n '360,450p;560,605p'
nl -ba MainPanel.c | sed -n '55,185p'
nl -ba Table.c | sed -n '250,410p'
nl -ba Process.h | sed -n '235,270p'
nl -ba pcp/InDomTable.c | sed -n '1,150p'
rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -- '*Screen*' Settings.c 2>/dev/null

Repository: htop-dev/htop

Length of output: 23767

Script executed:

rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -e 'InDomTable_new' -e 'Metric_iterate' --glob '*.[ch]' .
rg -n -F -e 'Action_follow' -e 'following' -- Action.c MainPanel.c ScreenManager.c
rg -n -F -e 'dynamic screen' -e 'Dynamic Screen' -e 'dynamic screens' -- README* docs pcp 2>/dev/null

Repository: htop-dev/htop

Length of output: 3891

Script executed:

nl -ba ScreensPanel.c | sed -n '60,235p;280,365p'
nl -ba pcp/PCPDynamicScreen.c | sed -n '300,405p'
nl -ba pcp/Metric.c | sed -n '150,220p'
nl -ba ScreenManager.c | sed -n '100,175p;280,450p'
rg --files pcp | grep -E 'screen|conf|README'
rg -n -F -e 'PCPDynamicScreen' -e 'dynamic screen' -e 'dynamic screens' -- pcp docs README.md

Repository: htop-dev/htop

Length of output: 33379

Script executed:

nl -ba ScreensPanel.c | sed -n '1,60p;234,280p;365,430p'
nl -ba ScreenManager.c | sed -n '225,282p'
nl -ba Table.c | sed -n '1,115p;115,255p'
nl -ba pcp/Instance.c | sed -n '1,180p'
nl -ba Machine.c | sed -n '50,120p'
nl -ba pcp/screens/devices | sed -n '1,100p'
rg -n -F -e 'Action_follow' -e 'following' -- docs/htop.1.in htop.1.in README.md 2>/dev/null

Repository: htop-dev/htop

Length of output: 30591

Do not transfer follow IDs between unrelated tables on Setup exit.

If Setup selects a different table, setActiveScreen can copy a follow ID from the previous table. A PCP instance with ID 1 can then cause the process table to follow PID 1. Preserve each table’s follow state without transferring IDs across tables.

Trivial · Maintainability & Code Quality · Poor tradeoff

Supported by static analysis

Script executed:

rg -n -i 'setup|dynamic.screen|activeTable|ssIndex|screen' test_spec.lua .github --glob '*.lua' --glob '*.yml' --glob '*.yaml' | head -n 180

Repository: htop-dev/htop

Length of output: 222

Script executed:

git diff 50919007b00ef657acdcea084c31a22b152c3799 e87dc28d5e7bebdb3adb6c582b5b745bf25fc11e -- Action.c
nl -ba Action.c | sed -n '1,180p'
rg --files -g '*test*' -g '*Test*' -g '*spec*' -g 'Makefile*' -g '*.yml' -g '*.yaml' -g 'configure.ac' -g 'meson.build' | sort
nl -ba test_spec.lua | sed -n '1,180p;350,460p'
rg -n -i -e 'dynamic.screen' -e 'test_spec' -e 'make check' -e 'busted' -e 'activeTable' -e 'ssIndex' -- .github Makefile.am configure.ac test_spec.lua 2>/dev/null || test "$?" -eq 1

Repository: htop-dev/htop

Length of output: 19867

Script executed:

rg -n -F -e 'setActiveScreen' -e 'Action_runSetup' -- Action.c
rg -n -F -e 'rebuildSettingsArray' -e 'ssIndex' -e 'activeTable' -- ScreensPanel.c Settings.c Machine.c Action.c
git ls-files | grep -Ei '(^|/)(test|tests|spec|specs)(/|[._-])|(^|/)[^/]*(test|spec)[^/]*$|(^|/)\.github/workflows/|(^|/)\.travis\.yml$'
rg -n -i -e 'test|busted|coverage|dynamic' -- Makefile.am configure.ac .travis.yml .github test_spec.lua default.htoprc 2>/dev/null
nl -ba test_spec.lua | sed -n '180,350p;458,800p'
nl -ba .travis.yml

Repository: htop-dev/htop

Length of output: 28821

Script executed:

nl -ba Action.c | sed -n '365,445p;585,600p'
nl -ba ScreensPanel.c | sed -n '90,235p;280,365p'
nl -ba Machine.c | sed -n '50,100p'
nl -ba .github/workflows/ci.yml | sed -n '1,175p;200,245p;410,455p'
nl -ba TESTPLAN | sed -n '1,200p'
git ls-files '*htoprc*'
if test -f default.htoprc; then nl -ba default.htoprc; else echo 'default.htoprc is not present in checkout'; fi
rg -n -i -e 'busted' -e 'test_spec\.lua' -e 'make check' -e 'installcheck' --glob '!test_spec.lua' --glob '!configure.ac' --glob '!Makefile.in' --hidden -g '!.git/**' . || test "$?" -eq 1

Repository: htop-dev/htop

Length of output: 31120

Add a regression test for Setup table synchronization.

If a screen backed by a different table is reordered in Setup, removing this call can leave settings->ss and host->activeTable out of sync. Add a test that reorders screens backed by different tables and checks the resulting pairing.

@pwilke pwilke Nov 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see two ways out of this, tell me what you prefer.

  • inline most of the setActiveScreen function here, without the follow ID transfers 
     Machine* host = st->host;
 if (!settings->ss->table)
    settings->ss->table = host->processTable;
 host->activeTable = settings->ss->table;

 bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
 MainPanel_setFunctionBar(st->mainPanel, readonly);

would replace the current setActiveScreen call.

  • extract the follow-ID-free part of setActiveScreen into another function, say setActiveScreenNoFollow, can call this latter function in Action_runSetup
static inline bool setActiveScreenNoFollow(Settings* settings, State* st, unsigned int ssIdx) {
   assert(settings->ssIndex == ssIdx);
   Machine* host = st->host;

   settings->ss = settings->screens[ssIdx];
   if (!settings->ss->table)
      settings->ss->table = host->processTable;
   host->activeTable = settings->ss->table;

   // set correct functionBar - readonly if requested, and/or with non-process screens
   bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
   MainPanel_setFunctionBar(st->mainPanel, readonly);

   return host->activeTable->following != -1;
}

static inline bool setActiveScreen(Settings* settings, State* st, unsigned int ssIdx) {
   // Save following state from current table before switching screens
   int following = st->host->activeTable->following;

   setActiveScreenNoFollow(settings, st, ssIdx);

   // Transfer following state to new table if it doesn't already have one
   Machine* host = st->host;
   if (following != -1 && host->activeTable->following == -1)
      host->activeTable->following = following;

   return host->activeTable->following != -1;
}

Which version do you prefer? Or a third one?

@coderabbitai coderabbitai Bot Nov 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Analysis chain

Script executed:

#!/bin/bash
for file in MainPanel.c ScreenManager.c ScreensPanel.c Table.c; do
  echo "===== outline: $file ====="
  ast-grep outline "$file" --items all --match 'ScreenManager|MainPanel|following|rebuildSettingsArray|Table' 2>/dev/null || true
done
echo '===== follow-state and reaction handling ====='
rg -n -C 5 -e 'HTOP_KEEP_FOLLOWING' -e 'following' -e 'rebuildSettingsArray' -- MainPanel.c ScreenManager.c ScreensPanel.c Table.c Action.c

Length of output: 25374

@pwilke I prefer the second option. It keeps screen and function-bar synchronization in one place and avoids transferring a follow ID on Setup exit. It also explicitly sets settings->ss from settings->screens[ssIdx].

The wrapper preserves the existing behavior for normal tab changes. Those changes can still transfer follow IDs between different tables, so this proposal addresses Setup exit only.

After the change, please verify the Setup reproduction with process and PCP screens, including that Setup does not copy the previous table’s follow ID to the newly active table.

You are interacting with an AI system.

@pwilke pwilke Nov 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

How about this new version of setActiveScreen

static inline bool setActiveScreen(Settings* settings, State* st, unsigned int ssIdx, bool transferFollow) {
   assert(settings->ssIndex == ssIdx);
   Machine* host = st->host;

   // Save following state from current table before switching screens
   int following = host->activeTable->following;

   settings->ss = settings->screens[ssIdx];
   if (!settings->ss->table)
      settings->ss->table = host->processTable;
   host->activeTable = settings->ss->table;

   // Transfer following state to the new table if requested and it has none.
   // Skipped across unrelated tables (e.g. on Setup exit): a row id is not
   // meaningful from one table kind to another.
   if (transferFollow && following != -1 && host->activeTable->following == -1)
      host->activeTable->following = following;

   // set correct functionBar - readonly if requested, and/or with non-process screens
   bool readonly = Settings_isReadonly() || (host->activeTable != host->processTable);
   MainPanel_setFunctionBar(st->mainPanel, readonly);

   return host->activeTable->following != -1;
}

It adds a flag saying whether follow should be transferred. There are 3 existing calls to patch with an extra true parameter, and our new call in Action_runSetup that would call setActiveScreen with transferFollow set to false.

I think this is better, what do you think?

@coderabbitai coderabbitai Bot Nov 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Analysis chain

Script executed:

#!/bin/bash
for file in Action.c MainPanel.c ScreenManager.c Table.c; do
  printf '\n===== %s outline =====\n' "$file"
  ast-grep outline "$file" --items all --match 'actionSetup|setActiveScreen|ScreenManager_run|following|MainPanel' 2>/dev/null || true
done
printf '\n===== follow state and reaction handling =====\n'
rg -n -C 6 -e 'HTOP_KEEP_FOLLOWING' -e 'following' -- Action.c MainPanel.c ScreenManager.c Table.c

Length of output: 23097

@pwilke Yes, I prefer this version. It keeps screen synchronization in one function and makes follow-ID transfer explicit.

Pass false on Setup exit and true at the three existing screen-switch calls. Also update the forward declaration. I would adjust the comment: transfers are skipped on Setup exit, but normal screen switches can still transfer IDs between unrelated tables.

After the change, please verify the Setup reproduction with process and PCP screens. Check that the destination table keeps its own follow state and does not receive the previous table’s ID.

You are interacting with an AI system.

}

static bool changePriority(MainPanel* panel, int delta) {
Expand Down Expand Up @@ -590,11 +597,11 @@ static Htop_Reaction actionLsof(State* st) {
if (!Action_writeableProcess(st))
return HTOP_OK;

const Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel);
if (!p)
Object* selected = Panel_getSelected((Panel*)st->mainPanel);
if (!selected || !Object_isA(selected, (const ObjectClass*) &Process_class))
return HTOP_OK;

assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class));
const Process* p = (const Process*) selected;

OpenFilesScreen* ofs = OpenFilesScreen_new(p);
InfoScreen_run((InfoScreen*)ofs);
Expand All @@ -608,11 +615,11 @@ static Htop_Reaction actionShowLocks(State* st) {
if (!Action_readableProcess(st))
return HTOP_OK;

const Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel);
if (!p)
Object* selected = Panel_getSelected((Panel*)st->mainPanel);
if (!selected || !Object_isA(selected, (const ObjectClass*) &Process_class))
return HTOP_OK;

assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class));
const Process* p = (const Process*) selected;

ProcessLocksScreen* pls = ProcessLocksScreen_new(p);
InfoScreen_run((InfoScreen*)pls);
Expand Down Expand Up @@ -655,11 +662,11 @@ static Htop_Reaction actionStrace(State* st) {
if (!Action_writeableProcess(st))
return HTOP_OK;

const Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel);
if (!p)
Object* selected = Panel_getSelected((Panel*)st->mainPanel);
if (!selected || !Object_isA(selected, (const ObjectClass*) &Process_class))
return HTOP_OK;

assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class));
const Process* p = (const Process*) selected;

TraceScreen* ts = TraceScreen_new(p);
bool ok = TraceScreen_forkTracer(ts);
Expand Down Expand Up @@ -927,11 +934,11 @@ static Htop_Reaction actionShowEnvScreen(State* st) {
if (!Action_readableProcess(st))
return HTOP_OK;

Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel);
if (!p)
Object* selected = Panel_getSelected((Panel*)st->mainPanel);
if (!selected || !Object_isA(selected, (const ObjectClass*) &Process_class))
return HTOP_OK;

assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class));
Process* p = (Process*) selected;

EnvScreen* es = EnvScreen_new(p);
InfoScreen_run((InfoScreen*)es);
Expand All @@ -945,11 +952,11 @@ static Htop_Reaction actionShowCommandScreen(State* st) {
if (!Action_readableProcess(st))
return HTOP_OK;

Process* p = (Process*) Panel_getSelected((Panel*)st->mainPanel);
if (!p)
Object* selected = Panel_getSelected((Panel*)st->mainPanel);
if (!selected || !Object_isA(selected, (const ObjectClass*) &Process_class))
return HTOP_OK;

assert(Object_isA((const Object*) p, (const ObjectClass*) &Process_class));
Process* p = (Process*) selected;

CommandScreen* cmdScr = CommandScreen_new(p);
InfoScreen_run((InfoScreen*)cmdScr);
Expand Down
Loading