Process tree loop is possible with some kernels #1017

Low-power · 2022-05-17T08:56:43Z

Some ptrace(2) implementations have a behavior of 're-parenting' the traced process to the tracer, which may causing problem with htop 'tree view'.

For example, to create such a loop on FreeBSD:

Use gdb(1) to attach the current shell (bash(1)) in a terminal (pts/46):

$ tty
/dev/pts/46
$ echo $$
3248
$ gdb -p $$
GNU gdb (GDB) 7.11.1 [GDB 7.11.1 for FreeBSD]
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
...
Attaching to process 3248
Reading symbols from /usr/local/bin/bash...(no debugging symbols found)...done.
Reading symbols from /lib/libncurses.so.8...(no debugging symbols found)...done.
Reading symbols from /usr/local/lib/libintl.so.8...(no debugging symbols found)...done.
Reading symbols from /lib/libc.so.7...(no debugging symbols found)...done.
Reading symbols from /libexec/ld-elf.so.1...(no debugging symbols found)...done.
[Switching to LWP 104322 of process 3248]
0x0000000800e409f8 in _wait4 () from /lib/libc.so.7
(gdb)

Inspect processes associated with the previous terminal using ps(1), in another terminal session:

$ tty
/dev/pts/42
$ ps -t pts/46 -o pid,ppid,comm
 PID PPID COMMAND
3248 6999 bash
6999 3248 gdb-7.11.1

Note the 2 processes has formed a loop.

htop crashing messages:

>>>>>>>>>> stderr output >>>>>>>>>>
Assertion failed: (Vector_size(this->displayList) == vsize), function ProcessList_buildTree, file ProcessList.c, line 316.

<<<<<<<<<< stderr output <<<<<<<<<<


FATAL PROGRAM ERROR DETECTED
============================
Please check at https://htop.dev/issues whether this issue has already been reported.
If no similar issue has been reported before, please create a new issue with the following information:
  - Your htop version: '3.2.0'
  - Your OS and kernel version (uname -a)
  - Your distribution and release (lsb_release -a)
  - Likely steps to reproduce (How did it happen?)
  - Backtrace of the issue (see below)

Error information:
------------------
A signal 6 (Abort trap) was received.

Setting information:
--------------------
htop_version=3.2.0;config_reader_min_version=3;fields=0 48 17 18 38 39 2 46 47 49 1;hide_kernel_threads=1;hide_userland_threads=0;shadow_other_users=0;show_thread_names=1;show_program_path=1;highlight_base_name=0;highlight_deleted_exe=1;highlight_megabytes=1;highlight_threads=1;highlight_changes=0;highlight_changes_delay_secs=5;find_comm_in_cmdline=1;strip_exe_from_cmdline=1;show_merged_command=0;header_margin=1;screen_tabs=0;detailed_cpu_time=0;cpu_count_from_one=1;show_cpu_usage=1;show_cpu_frequency=0;show_cpu_temperature=0;degree_fahrenheit=0;update_process_names=0;account_guest_in_cpu_meter=0;color_scheme=0;enable_mouse=1;delay=15;hide_function_bar=0;header_layout=two_50_50;column_meters_0=AllCPUs;column_meter_modes_0=1;column_meters_1=Tasks LoadAverage Uptime Blank Memory Swap;column_meter_modes_1=2 2 2 2 1 1;tree_view=1;sort_key=47;tree_sort_key=0;sort_direction=1;tree_sort_direction=1;tree_view_always_by_pid=0;all_branches_collapsed=0;screen:Main=PID USER PRIORITY NICE M_VIRT M_RESIDENT STATE PERCENT_CPU PERCENT_MEM TIME Command;.sort_key=PERCENT_MEM;.tree_sort_key=PID;.tree_view=1;.tree_view_always_by_pid=0;.sort_direction=1;.tree_sort_direction=1;.all_branches_collapsed=0;

Backtrace information:
----------------------
0x407bd0 <print_backtrace+0x14> at /usr/home/WHR/src/htop-dev-htop/htop
0x407f60 <CRT_handleSIGSEGV+0xaf> at /usr/home/WHR/src/htop-dev-htop/htop

To make the above information more practical to work with, please also provide a disassembly of your htop binary. This can usually be done by running the following command:

   objdump -d -S -w `which htop` > ~/htop.objdump

Please include the generated file in your report.
Running this program with debug symbols or inside a debugger may provide further insights.

Thank you for helping to improve htop!

This commit adds a rescan logic, which will rescan for looped processes once there appears to be one or more loops.

Low-power · 2022-05-17T09:01:13Z

Note this ptrace(2) behavior of FreeBSD kernel is documented in the man page:

For the duration of the tracing session, the traced process will be ``re-parented'', with its parent process ID (and resulting behavior) changed to the tracing process.

tanriol · 2022-05-17T09:53:28Z

ProcessList.c

+   if(display_size < vsize) {
+      for (int i = 0; i < vsize; i++) {
+         Process* process = (Process*)Vector_get(this->processes, i);
+         if(!process->isRoot && Vector_indexOf(this->displayList, process, Process_pidEqualCompare) == -1) {


Note that Vector_indexOf takes linear time, so this special case bumps tree build complexity to O(n^2).

I'd suggest marking all processes as unseen in the root finding phase before sort by setting tree_depth to a guard value like UINT_MAX and checking processes for that guard value instead.

You may also want to chase to loop in case the process you encounter first is not one of the loop processes, but a descendant of one of them.

Low-power · 2022-05-17T12:05:04Z

I'd suggest marking all processes as unseen in the root finding phase before sort by setting tree_depth to a guard value like UINT_MAX and checking processes for that guard value instead.

Good idea; however I think setting tree_depth to 0 is enough, because for a process having both !process->isRoot && process->tree_depth == 0 is enough to determine that it is out of the newly built tree(s).

You may also want to chase to loop in case the process you encounter first is not one of the loop processes, but a descendant of one of them.

I currently have no idea how to handle it efficiently.

tanriol · 2022-05-17T12:12:30Z

You may also want to chase to loop in case the process you encounter first is not one of the loop processes, but a descendant of one of them.

I currently have no idea how to handle it efficiently.

Start from the first missing process you see. As long as the parent of the current process has not been marked as visited, you mark the current process as visited and switch to the parent. When you see that the parent has already been marked as seen, both the current process and the parent are known to belong to the loop body, so you can start building the tree from either. Continue the scan for missing processes in case there are more missing processes.

This discovery step is still O(n) for all the processes because every process can be marked as visited at most once (and thus dominated by O(n * log(n)) for building the trees).

tanriol · 2022-05-17T14:40:19Z

ProcessList.c

+            do {
+               // Make sure we break at the loop itself, not a descendant of it.
+               process->tree_depth = 1;
+               Process *parent = ProcessList_findProcess(this, process->ppid);


I think this should use Process_getParentPid for consistency with the tree sort code.

tanriol · 2022-05-17T14:59:24Z

ProcessList.c

+               if(!parent) break;
+               process = parent;
+               j--;
+            } while(!process->isRoot && !process->tree_depth);


Do I understand correctly that all three of j > 0, if(!parent) break; and !process->isRoot in this while are sanity checks that should never trigger if the logic of this code is correct? If so, they likely should have assertions so that logic errors cannot be introduced unnoticed.

Also note that, as you exit this loop on process->tree_depth != 0, you'll need to reset it to 0 again for the newfound root.

tanriol · 2022-05-17T15:04:13Z

ProcessList.c

+   // here.
+   int display_size = Vector_size(this->displayList);
+   if(display_size < vsize) {
+      for (int i = 0, j = vsize - display_size; i < vsize && j > 0; i++) {


j looks kinds meaningless - you don't try to track it in the inner loop, and it differs from vsize - display_size either as you don't know the number of nodes added to the displayList.

Low-power · 2022-05-17T15:39:09Z

I think this should use Process_getParentPid for consistency with the tree sort code.

Yep, I missed this.

The outer for loop iterates for each looped process tree, while the inner do loop iterates for each process in a tree to findout where to cut the looped tree.

j counts the remaining lost nodes; when it becomes 0, there shouldn't be any lost nodes, so break out the outer loop to prevent wasting time to find more lost nodes.

j-- is inside the inner loop bacause all processes iterated here are expected to be added to displayList later by ProcessList_buildTreeBranch. Yes, this isn't perfect, as it is also possible to have an deeper descendant didn't count to j but picked up by ProcessList_buildTreeBranch, but it was used in hope to speed up things a litttle bit.

Other checks you mentioned are sanity checks.

tanriol · 2022-05-17T16:17:05Z

j counts the remaining lost nodes; when it becomes 0, there shouldn't be any lost nodes, so break out the outer loop to prevent wasting time to find more lost nodes.

How about something like if(Vector_size(this->displayList) == vsize) break; after building the branch for the current loop instead?

Low-power · 2022-05-17T16:34:36Z

How about something like if(Vector_size(this->displayList) == vsize) break; after building the branch for the current loop instead?

Makes sence, Vector_size is cheap.

Low-power · 2022-05-17T16:45:23Z

Added assert(3)s.

tanriol · 2022-05-17T16:52:15Z

Looks good to me from the content POV. Sorry, I'm not familiar enough with the styleguide to review code style, so I'll leave that to @BenBE or someone else from the main team.

BenBE

The nesting of this section of the code is somewhat borderline (as is most of that part of the existing code in this area) in regards to indentation and complexity. So while most comments seem to indicate a minor white space change, they are mostly for completeness. ;-)

Furthermore some of the checks used to reduce the number of loop iterations may be worth pulling out to the for loop iterating over the around lines 320 or 325.

Another performance consideration affecting this code affects the process parent lookup around line 328: Have you checked if looking up that process object using the process hashtable is viable? If so, search might be considerably faster, as no linear search has to be performed comparing process IDs.

BenBE · 2022-05-17T17:26:15Z

ProcessList.c

         Vector_add(this->displayList, process);
         ProcessList_buildTreeBranch(this, process->pid, 0, 0, process->showChildren);
         continue;
      }
   }

+   // Under some ptrace(2) implementations, a process ptrace(2)-attaching a
+   // parent process in its process tree will causing the traced process to


Suggested change

// parent process in its process tree will causing the traced process to

// parent process in its process tree will cause the traced process to

BenBE · 2022-05-17T17:26:53Z

ProcessList.c

+   // process tree. In this case build separated tree(s) for loop(s) left
+   // here.
+   int display_size = Vector_size(this->displayList);
+   if(display_size < vsize) {


Suggested change

if(display_size < vsize) {

if (display_size < vsize) {

BenBE · 2022-05-17T17:27:04Z

ProcessList.c

+   if(display_size < vsize) {
+      for (int i = 0; i < vsize; i++) {
+         Process* process = (Process*)Vector_get(this->processes, i);
+         if(!process->isRoot && process->tree_depth == 0) {


Suggested change

if(!process->isRoot && process->tree_depth == 0) {

if (!process->isRoot && process->tree_depth == 0) {

I think it may be worth considering to invert this condition to allow skipping the entire loop body that follows and thus reduce the overall indentation level.

BenBE · 2022-05-17T17:28:06Z

ProcessList.c

+               process->tree_depth = 1;
+               Process *parent = ProcessList_findProcess(this, Process_getParentPid(process));
+               assert(parent != NULL);
+               if(!parent) break;


Suggested change

if(!parent) break;

if (!parent)

break;

BenBE · 2022-05-17T17:28:22Z

ProcessList.c

+               if(!parent) break;
+               process = parent;
+               assert(!process->isRoot);
+            } while(!process->isRoot && !process->tree_depth);


Suggested change

} while(!process->isRoot && !process->tree_depth);

} while (!process->isRoot && !process->tree_depth);

BenBE · 2022-05-17T17:29:45Z

ProcessList.c

+            process->indent = 0;
+            Vector_add(this->displayList, process);
+            ProcessList_buildTreeBranch(this, process->pid, 0, 0, process->showChildren);
+            if(Vector_size(this->displayList) == vsize) break; // No more loop exist.


Suggested change

if(Vector_size(this->displayList) == vsize) break; // No more loop exist.

if (Vector_size(this->displayList) == vsize)

break; // No more loops exist.

BenBE · 2022-05-17T17:33:06Z

ProcessList.c

+            ProcessList_buildTreeBranch(this, process->pid, 0, 0, process->showChildren);
+            if(Vector_size(this->displayList) == vsize) break; // No more loop exist.
+         }
+         // There appears to have more loop(s) left, trying to continue find.


I think this explanatory comment somewhat confuses in the context of the previous check one nesting layer in.

tanriol · 2022-05-17T17:48:06Z

Another performance consideration affecting this code affects the process parent lookup around line 328: Have you checked if looking up that process object using the process hashtable is viable? If so, search might be considerably faster, as no linear search has to be performed comparing process IDs.

ProcessList_findProcess already uses hashtable, so this one should be fine.

tanriol · 2022-05-17T18:33:16Z

Trying this PR locally and there seems to be some problem I don't fully understand yet.

tanriol · 2022-05-17T18:42:58Z

@Low-power Sorry, I missed that you're missing the other part - ProcessList_buildTreeBranch does not have a built-in check for "this process hasn't been added yet", so it would go into infinite recursion as we don't re-sort the process list. As you mark these new roots as roots, skipping any process with isRoot should do the trick.

Low-power · 2022-05-18T04:23:38Z

Sorry, I missed that you're missing the other part - ProcessList_buildTreeBranch does not have a built-in check for "this process hasn't been added yet", so it would go into infinite recursion as we don't re-sort the process list. As you mark these new roots as roots, skipping any process with isRoot should do the trick.

I don't think I understand this, but shouldn't marking a node as root (isRoot = true) be enough to prevent a infinite recursion?

tanriol · 2022-05-18T05:41:59Z

Sorry, I missed that you're missing the other part - ProcessList_buildTreeBranch does not have a built-in check for "this process hasn't been added yet", so it would go into infinite recursion as we don't re-sort the process list. As you mark these new roots as roots, skipping any process with isRoot should do the trick.

I don't think I understand this, but shouldn't marking a node as root (isRoot = true) be enough to prevent a infinite recursion?

Yeah, that's the check I'm suggesting to add. Before this PR that was not needed because of a special case in compareProcessByKnownParentThenNatural sorting all roots as if their parent PID was 0 instead.

BenBE · 2022-06-01T16:13:28Z

@Low-power Could you please update your PR to fix the issues that people reported? TIA.

tanriol reviewed May 17, 2022

View reviewed changes

BenBE added bug 🐛 Something isn't working FreeBSD 👹 FreeBSD related issues labels May 17, 2022

Low-power force-pushed the break-looped-proc-tree branch 2 times, most recently from 0bb1bbf to 67cba08 Compare May 17, 2022 11:57

Low-power force-pushed the break-looped-proc-tree branch from 67cba08 to fe4ccb2 Compare May 17, 2022 14:35

tanriol reviewed May 17, 2022

View reviewed changes

Low-power force-pushed the break-looped-proc-tree branch from fe4ccb2 to a28fdf1 Compare May 17, 2022 15:42

Low-power force-pushed the break-looped-proc-tree branch from a28fdf1 to 52104ff Compare May 17, 2022 16:33

Fix assert(3)ion failure due to process tree loop in some rare cases

1df5d1b

Low-power force-pushed the break-looped-proc-tree branch from 52104ff to 1df5d1b Compare May 17, 2022 16:42

BenBE requested changes May 17, 2022

View reviewed changes

tanriol mentioned this pull request Jan 16, 2023

Add options to display parents and children processes of filtered pro… #1139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process tree loop is possible with some kernels #1017

Process tree loop is possible with some kernels #1017

Low-power commented May 17, 2022

Low-power commented May 17, 2022

tanriol May 17, 2022

tanriol May 17, 2022

Low-power commented May 17, 2022

tanriol commented May 17, 2022 •

edited

Loading

tanriol May 17, 2022

tanriol May 17, 2022

tanriol May 17, 2022

tanriol May 17, 2022

Low-power commented May 17, 2022

tanriol commented May 17, 2022

Low-power commented May 17, 2022

Low-power commented May 17, 2022

tanriol commented May 17, 2022

BenBE left a comment

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

BenBE May 17, 2022

tanriol commented May 17, 2022

tanriol commented May 17, 2022

tanriol commented May 17, 2022

Low-power commented May 18, 2022 •

edited

Loading

tanriol commented May 18, 2022

BenBE commented Jun 1, 2022

	// parent process in its process tree will causing the traced process to
	// parent process in its process tree will cause the traced process to

	if(!process->isRoot && process->tree_depth == 0) {
	if (!process->isRoot && process->tree_depth == 0) {

	} while(!process->isRoot && !process->tree_depth);
	} while (!process->isRoot && !process->tree_depth);

-            if(Vector_size(this->displayList) == vsize) break; // No more loop exist.
+            if (Vector_size(this->displayList) == vsize)
+               break; // No more loops exist.

Process tree loop is possible with some kernels #1017

Are you sure you want to change the base?

Process tree loop is possible with some kernels #1017

Conversation

Low-power commented May 17, 2022

Low-power commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Low-power commented May 17, 2022

tanriol commented May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Low-power commented May 17, 2022

tanriol commented May 17, 2022

Low-power commented May 17, 2022

Low-power commented May 17, 2022

tanriol commented May 17, 2022

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanriol commented May 17, 2022

tanriol commented May 17, 2022

tanriol commented May 17, 2022

Low-power commented May 18, 2022 • edited Loading

tanriol commented May 18, 2022

BenBE commented Jun 1, 2022

tanriol commented May 17, 2022 •

edited

Loading

Low-power commented May 18, 2022 •

edited

Loading