Jim Warner [Mon, 25 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: parent total cpu includes collapsed children, pgm
Now, when a parent's children have been collapsed, the
cpu used by those unseen tasks will disappear no more.
Instead such tics will be added to the parent's total.
[ if one wished a return to the 'land of lost tics', ]
[ the '#define TREE_VCPUOFF' directive is available. ]
------------------------------------------------------
Note: With collapsible parents now displaying children
cpu usage, it will eventually be noticed the cpu stats
for the summary area and task areas often vary widely.
It's worth a reminder that for top's summary area each
individual cpu and the cpu summary is limited to 100%,
regardless of how many tics a linux kernel may export.
An individual task is limited to 100% times the number
of threads. But, in no case will cpu usage ever exceed
100% times total number of processors. Such limits are
further reduced under 'Solaris' mode ('I' toggle off).
In this mode, a task cpu usage will never exceed 100%.
These limits will now also apply to collapsed parents.
In addition to those influences, results are subjected
to kernel timer sampling anomalies and the distortions
inherent in a small sample size, made worse by smaller
delay intervals. Often there is just 1 or 2 tics for a
few tasks at smaller intervals such as: 1/10th second.
Anyway, should questions on this subject arise, a good
starting point, beyond the reminders above, is the 1st
link listed below. Those other links were derivatives.
Reference(s):
. from the kernel documentation
https://www.kernel.org/doc/Documentation/cpu-load.txt
. as mentioned in the above kernel documentation
https://lkml.org/lkml/2007/2/12/6
. from above, with many more links on the subject
https://www.boblycat.org/~malc/apc/
Signed-off-by: Jim Warner <james.warner@comcast.net>
top: parent total cpu includes collapsed children, pgm
Jim Warner [Sun, 24 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: a refactor to prepare for including collapsed cpu
So that the impact (minimal) of the next commit can be
isolated, this commit just involves a little renaming,
reformat plus a refactor of some proc_t pointer logic.
[ renaming, relocation and changes to 'user_matched' ]
[ wasn't strictly necessary, but now mirrors newlib. ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sat, 23 Jun 2018 17:00:00 +0000 (12:00 -0500)]
top: a few tweaks for those scrolling (mostly) changes
This patch just addresses some edge cases with respect
to 'unseen' tasks. Given the ability to preserve other
filters in the rcfile, it's entirely possible the very
first task(s) may not be visible at top startup. Also,
when switching between windows ('a'/'w') we should try
to always position its row #1 on some visible process.
Lastly, a window might have *NO* visible tasks at all.
Therefore, protect 'window_hlp' from an infinite loop.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Robert Bowmaker [Sat, 23 Jun 2018 08:07:47 +0000 (08:07 +0000)]
Fix error in vmstat documentation.
Inspection of vmstat.c and experimentation with the binary itself
both confirm that the units of the swap (si/so) fields are
controlled by the --unit option.
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: normalize vertical scrolling for hidden processes
To my knowledge, nobody has ever complained about some
anomalies when scrolling vertically if tasks should be
hidden from view. This can happen with the user filter
('u/U') or other filter ('o/O') features. And although
some tasks are not shown, they still impact scrolling.
This is most apparent when that scroll coordinates msg
is on ('C') & up/down arrow keys used (vs. pgup/pgdn).
Now that we can collapse/expand forked children, there
is a potential for yet more of those hidden processes.
So this commit normalizes vertical scrolling providing
an expected behavior. In other words, the up/down keys
skip the unseen tasks to reposition on a visible task.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactor the 'scroll coordinates' message support
This patch is simply preparation for upcoming vertical
scrolling enhancements. With those changes, it will be
impossible to predict what the beginning task position
should be at the time the message is currently issued.
This patch will allow such a message to be shown after
the individual windows' tasks have all been displayed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
The issue cited below really dealt with preserving the
'Other filter' criteria in the rcfile. But as an aside
the htop 'F6' feature (collapsed children) was raised.
I took that as an implied challenge and decided to try
implementing a similar feature in top. So, this commit
will now provide a brand new forest view toggle ('^V')
which will be used to collapse/expand forked children.
[ this patch will also lead to additional patches in ]
[ support of more rational vertical scrolling, since ]
[ many more tasks might now be hidden in some window ]
Reference(s):
. where this secondary issue was raised
https://gitlab.com/procps-ng/procps/issues/99
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Tue, 19 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: to prepare for collapse, move forest view support
In anticipation of a new collapsible child feature, we
will have to make some forest view variables available
to that 'keys_task()' function. This commit just moves
the forest view logic ahead of tertiary input support.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sun, 17 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: 'other filters' saved with config file, pgm logic
Well, after the rearranging and refactoring, all those
active 'other filter' entries for each window will now
be preserved in the user's configuration file via 'W'.
For raising the issue below, thanks to Marco Ippolito.
Jim Warner [Sun, 17 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactored some more peripheral 'inspect' support
These modifications are being made now in anticipation
of some coming 'other filter' config file changes. Our
entries must be written last to the rc file since that
is where the users have been told to 'echo' additions.
Therefore, that 'config_insp' function must be adapted
to anticipate a passed buffer that was already primed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sun, 17 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactored some of that 'other filtering' support
If we are to support preserving 'other filter' entries
in the rcfile, then the current logic setting up those
osel entries for a WIN_t must be shareable for startup
and when interacting with a user. So, this commit just
repositions this current code in a shareable function.
[ along the way, we give the prior guy a proper name ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sun, 17 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: reposition some of that 'other filtering' support
When we get around to saving that 'Other Filter' stuff
in the rcfile, we'll need access to the Fieldstab plus
the justify_pad() function. So this commit repositions
two 'osel' functions in anticipation of adding 1 more.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sun, 17 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactor some of that configuration files support
The 'config_file()' function was getting a little long
in the tooth, so this commit simply renames/rearranges
some stuff anticipating 'other filters' in the rcfile.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 6 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: force return to row 1 for thread mode transitions
This program does a good job of policing that vertical
scrolled position, ensuring that total tasks are never
exceeded. However, during transitions from thread mode
to normal task mode (the 'H' toggle) that wasn't true.
And while there was no real harm done, it did make the
use of up/down arrow keys "appear" disabled especially
if that scroll message was not displayed ('C' toggle).
This patch simply forces a return to row #1 whenever a
user toggles that display between thread & task modes.
Signed-off-by: Jim Warner <james.warner@comcast.net>
As it turns out, the very first entry in the 'iokey()'
tinfo_tab was preventing the proper translation of the
simulated PgUp/PgDn keys (ctrl+meta+k/j). Ignoring the
tortured history behind the most recent change to that
entry, this patch restores the previous value and once
again properly translates these particular keystrokes.
Signed-off-by: Jim Warner <james.warner@comcast.net>
The current checks allow out-of-range values (for example, if
getenv/atoi returns ~-2GB, maxcmd becomes ~+2GB after the subtraction).
This is not a security problem, none of this is under an attacker's
control.
Jim Warner [Mon, 4 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: provide the means to exploit a 256-color terminal
With the Qualys security audit, we began to harden our
treatment of the top rcfile. In particular, the values
read were checked so as to prevent some malicious user
from editing it in order to achieve an evil objective.
However when it came to colors I was surprised to find
that at least one user edited the rcfile for 256-color
support. Unfortunately, our new checks prevented this.
So this commit will provide the means to exploit those
extra colors with no need to manually edit the rcfile.
Jim Warner [Fri, 1 Jun 2018 05:00:00 +0000 (00:00 -0500)]
library: tweak that change to 'supgrps_from_supgids()'
It's good that those Qualys folks were looking over my
shoulder. They suggested a change to that commit shown
below. This improvement was obviously a better choice.
Following that patch referenced below, the top SUPGRPS
field would produce a segmentation fault and ps SUPGRP
would often show "(null)". Such problems resulted from
some faulty logic in the status2proc() routine dealing
with 'Groups' (supgid) which served as a source field.
For many processes the original code produced an empty
string which prevented conversion to the expected "-".
Moreover, prior to release 3.3.15 such an empty string
will become 0 after strtol() which pwcache_get_group()
translates to 'root' yielding very misleading results.
So, now we'll check for empty '/proc/#/status/Groups:'
fields & consistently provide a "-" value for callers.
[ we'll also protect against future problems in that ]
[ new qualys logic by always ensuring valid 'supgrp' ]
[ pointers - logic which revealed our original flaw! ]
Reference(s):
. original qualys patch
0071-proc-readproc.c-Harden-supgrps_from_supgids.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Fri, 25 May 2018 05:00:00 +0000 (00:00 -0500)]
top: sanitized some potentially corrupt 'Inspect' data
This guards against rcfile 'Inspect' entries which may
include non-printable characters. While this shouldn't
occur, we have no real control over those crazy users.
[ and, while such data can't be used maliciously, it ]
[ does adversely impact such a user's screen display ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Thu, 24 May 2018 05:00:00 +0000 (00:00 -0500)]
top: prevent buffer overruns in 'inspection_utility()'
When a Qualys patch was reverted as being unwarranted,
1 specific problem their patch had, in fact, prevented
was re-introduced. This patch corrects that oversight.
Jim Warner [Wed, 23 May 2018 05:00:00 +0000 (00:00 -0500)]
top: add another field sanity check in 'config_file()'
Until the Qualys security audit I had never considered
it a possibility that some malicious person might edit
the top config file to achieve some nefarious results.
And while the Qualys approach tended to concentrate on
the symptoms from such an effort, subsequent revisions
more properly concentrated on startup and that rcfile.
This commit completes those efforts with 1 more field.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sun, 20 May 2018 05:00:00 +0000 (00:00 -0500)]
top: don't mess with groff line length in man document
I've long since forgotten why the attempt to influence
groff line lengths was made. However, I did receive an
email regarding problems formatting postscript output.
Hopefully this patch will eliminate any such problems.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Craig Small [Sat, 19 May 2018 12:10:11 +0000 (22:10 +1000)]
ps: Increase command selection field to 64
The library now presents command names up to 64 characters, in line with
the kernel changes. ps command name selection (the -C option) now also
is 64 characters long.
Jim Warner [Sat, 19 May 2018 05:00:00 +0000 (00:00 -0500)]
top: just respond to the increased command name length
The command name for running tasks is displayed by top
in a variable length field, so the increase from 16 to
64 bytes was not a problem. However, there's one place
where top is sensitive to length - insp_view_choice().
So, this patch just bumps a buffer used to display it.
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Prevent out-of-bounds writes in PUFF(). __Tweaked
This commit moves some overhead to the Batch mode path
where it's needed. And given the new 'else if' test we
can delete some now redundant logic in the other path.
This patch addresses a potential (but unlikely) buffer
overflow by reducing, if necessary, a memcpy length by
3 bytes to provide for an eol '\0' and 2 unused buffer
positions which also might receive the '\0' character.
[ note to future analysis tool: just because you see ]
[ binary data being manipulated in the routine, that ]
[ doesn't mean such function was passed binary data! ]
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Fix out-of-bounds read/write in show_... REVERTED
I'm reverting this patch to prepare for some alternate
solution. In that solution I will address point #1 but
point #2 is based on a wrong assumption. There will be
no binary data ever found in the 'glob' passed to this
show_special() function. It is now always simple text.
------------------------------------------------ original commit message
This patch fixes two problems:
1/ In the switch case 0, if sub_end is at the very end of lin[], the two
null-byte writes are off-by-two (a stack-based buffer overflow). Replace
this end-of-string "emulation" with an equivalent test on ch (and then
goto/break out of the loop).
2/ "sub_end += 2" jumps over the null-byte terminator in lin[] if the
line contains a raw (without a tilde) \001-\010 character. Detect such a
null-byte terminator and goto/break out of the loop.
Note: in the case of a raw \001-\010 character, the character at
"sub_end + 1" is never processed (it is skipped/jumped over); this is
not a security problem anymore (since 2/ was fixed), so we decided not
to change this behavior, for backward-compatibility.
------------------------------------------------------------------------
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: prevent buffer overflow potential in all routines
Whereas that original patch (since reversed) addressed
some symptoms related to manually edited config files,
this solution deals with root causes. And it goes much
beyond any single top field by protecting all of top's
fields. Henceforth, a duplicated field is not allowed.
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Prevent buffer overflow in calibrate_... REVERTED
Here, again, we have an example of attacking a problem
by addressing the symptoms. And that assertion made in
the original commit message is true if only if someone
had manually (maliciously) edited the top config file.
So let's reverse the original patch & thus prepare for
a proper solution addressing the cause, not a symptom.
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Prevent integer overflows in procs_re... REVERTED
That patch referenced below is being reverted because:
. By design, no other top macro looks like a function.
Instead, they all contain some minimal capitalization.
The 'grow_by_size' macro stands out like a sore thumb.
. We would need to approach 400+ million tasks for for
the 1st addressed problem to produce integer overflow.
. And a 2nd check against SSIZE_MAX remains a mystery.
Me thinks a system on which top is running will suffer
ENOMEM before we need to worry about integer overflow.
Rather than validate the window's 'sortindx' each time
it was referenced (as was done in the patch below), we
now ensure the validity just once when the config file
is read. Thereafter, a running top will police bounds.
Here's yet another example of dealing with a potential
problem at the symptom level, instead of addressing it
at the source. So, we will reverse that original patch
referenced below in preparation for a proper solution.
[ at the least, this ugly code should have used that ]
[ existing MAXTBL macro, making it a little prettier ]
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: other graph_cpus, graph_mems, and summ_mscale fix
This patch replaces an original patch referenced below
which has now been reversed. We now validate variables
'graph_cpus', 'graph_mems' and 'summ_mscale' just once
at startup. Thereafter, top enforces the proper range.
[ we afford the same treatment to that 'task_mscale' ]
[ variable, which was ignored in the original patch. ]
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Check graph_cpus, graph_mems, and sum... REVERTED
The variables graph_cpus, graph_mems & summ_mscale are
all well managed in a running top. They were, however,
each vulnerable to tampering via the rcfile. So rather
than continually addressing the symptoms, we'll attack
the root cause just once at startup in the next patch.
Jim Warner [Fri, 18 May 2018 05:00:00 +0000 (00:00 -0500)]
top: Do not default to the cwd in configs_r... Tweaked
While it's only documented (so far) in commit text and
an occasional email I've tried to maintain some coding
standards primarily for reference/navigation purposes.
They also served, I felt, as useful mental challenges.
Someday I will get around to formerly documenting them
but in the meantime here are the ones for this commit:
. functions are grouped into logical (i hope) sections
. functions & sections are ordered to avoid prototypes
. function names are alphabetical within every section
. all functions & sections must be referenced in top.h
This patch just attempts to honor the above standards,
while also covering this new behavior in the man page.
[ please note that the net result of these 2 patches ]
[ is simply to avoid pathname truncations should our ]
[ limit of 1024 be exceeded. they do not have a role ]
[ in solving the 'local privilege escalation' issue. ]
[ and we can never prevent a user from setting their ]
[ HOME var to a directory writable by some attacker! ]
[ the only real protection for that CVE-2018-1122 is ]
[ those soon to be enhanced rcfile integrity checks, ]
[ achieved through several of the following patches. ]
Craig Small [Fri, 18 May 2018 22:14:06 +0000 (08:14 +1000)]
pgrep: Remove >15 warning
As comm length can be longer than 15 characters with newer kernels, it
doesn't make sense to have a warning when you make the match string
longer than this.
As a side-effect, it removes the false-positive you got when you used
long regex matches (see issue #92 )
Craig Small [Fri, 18 May 2018 22:04:19 +0000 (08:04 +1000)]
library: Increase comm length to 64
For many years, the comm length has been set to 16. Previously to that
it was 8. This means for things like kworkers they all have very cryptic
names. The kernel is now going to increase this size to 64, so the
procps library will follow this length increase.
System tools may also increase their default length to 64, or keep it at
16; there is only so much screen real estate.
1/ Because snprintf() returns "the number of characters (excluding the
terminating null byte) which would have been written to the final string
if enough space had been available", _eol may point past the end of _str
and write out-of-bounds (in Batch mode).
2/ _eol is never checked against _str, so "while (*(--_eol) == ' ');"
may point _eol below _str and write out-of-bounds (in Batch mode).
3/ Sanity-check Pseudo_row to protect the strcpy().
top: Fix out-of-bounds read/write in show_special().
This patch fixes two problems:
1/ In the switch case 0, if sub_end is at the very end of lin[], the two
null-byte writes are off-by-two (a stack-based buffer overflow). Replace
this end-of-string "emulation" with an equivalent test on ch (and then
goto/break out of the loop).
2/ "sub_end += 2" jumps over the null-byte terminator in lin[] if the
line contains a raw (without a tilde) \001-\010 character. Detect such a
null-byte terminator and goto/break out of the loop.
Note: in the case of a raw \001-\010 character, the character at
"sub_end + 1" is never processed (it is skipped/jumped over); this is
not a security problem anymore (since 2/ was fixed), so we decided not
to change this behavior, for backward-compatibility.
- Make sure i is at least 1 before "i - 1" and "--i".
- Initialize endpflg (to 0, as it was originally, since it is static)
before the "for" loop (the "break" may leave endpflg uninitialized,
for example).
top: Prevent buffer overflow in calibrate_fields().
pflgsall[] can contain PFLAGSSIZ = 100 elements, each iteration of the
loop can write 3 elements to pflgsall[], and there are EU_MAXPFLGS = 58
iterations: a buffer overflow (it can be triggered via the configuration
file, for example, by filling "fieldscur" with the "sortindx" flag).
The safety of the critical function task_show() depends on the sanity of
Screen_cols. Just copy the tests on w_cols to Screen_cols (from the same
function adj_geometry()).
Several of these buffer overflows can actually be triggered (through the
configuration file for example): in config_file(), inspection_utility(),
and show_special().
The default action for SIGURG is to ignore the signal, for example.
This is very similar to the patch "ps/display.c: Always exit from
signal_handler()."
Every time sortindx is used as an index, or loaded from the
configuration file. Otherwise it leads to out-of-bounds reads and
arbitrary code execution.
Otherwise they may lead to out-of-bounds writes (snprintf() returns the
number of characters which would have been written if enough space had
been available).
Also, make sure buf is null-terminated after COLPLUSCH has been written.
If the HOME environment variable is not set, or not absolute, use the
home directory returned by getpwuid(getuid()), if set and absolute
(instead of the cwd "."); otherwise, set p_home to NULL.
To keep the changes to a minimum, we rely on POSIX, which requires that
fopen() fails with ENOENT if the pathname (Rc_name) is an empty string.
This integrates well into the existing code, and makes write_rcfile()
work without a change.
Also, it makes the code in configs_read() easier to follow: only set and
use p_home if safe, and only set Rc_name if safe (in all the other cases
it is the empty string, and the fopen() calls fail). Plus, check for
snprintf() truncation (and if it happens, reset Rc_name to the empty
string).
Important note: top.1 should probably be updated, since it mentions the
fallback to the current working directory.
Check the return value of sscanf() to make sure that all input items are
properly initialized.
In extended mode (x_option), one_proc() loads the values of start and
perms during one iteration of the while loop, and displays them during
one of the following iterations, but start and perms are variables local
to the while loop: move them out of the while loop, to the beginning of
the function.
Also, display a mapping only if cp2 is properly initialized; otherwise
(for example), mappings that do not belong to a selected range are
displayed, and with a NULL mapping name:
If "cp = strrchr(mapbuf_b, '/')" then this function returns, and
otherwise there is no '/' in mapbuf_b and "cp = strchr(mapbuf_b, '/')"
is always false: remove this second block, since it is never entered.
Also, constify a few things in this function.
In the headers, the space was misplaced; for example, "pmap -XX $$"
outputs "VmFlagsMapping" (without a space). Use justify_print() instead
of printf().
There was also an extra space in the output, because vmflags[] (from the
"VmFlags:" line) always ends with a space. Overwriting this last space
with a null byte fixes this misalignment.
vmflags[] is a 27*(2+1)=81 char array, but there are 30 flags now (not
27), and even with 27 flags this was an off-by-one overflow (the kernel
always outputs a flag with "%c%c ", so the last +1 is for a space, not
for the terminating null byte). Protect vmflags[] with a maximum field
width, as in the surrounding sscanf() calls.
Otherwise "the contents of the array remain unchanged and a null pointer
is returned" or "the array contents are indeterminate and a null pointer
is returned".
proc/readproc.c: Work around a design flaw in readeither().
readeither() caches (in new_p) a pointer to the proc_t of a task-group
leader, but readeither()'s callers can do pretty much anything with the
proc_t structure passed to and/or returned by this function. For
example, they can 1/ free it or 2/ recycle it (by passing it to
readeither() as x).
1/ leads to a use-after-free, and 2/ leads to unexpected behavior when
taskreader()/simple_readtask() is called with new_p equal to x (this is
not a theoretical flaw: 2/ happens in readproctab3() when want_task()
returns false and p is a group leader).
As a workaround, we keep a copy of new_p's first member (tid) in static
storage, and the next times we enter readeither() we check this "canary"
against the tid in new_p: if they differ, we reset new_p to NULL, which
forces the allocation of a new proc_t (the new "leader", or reference).
This always detects 2/ (because free_acquired(x,1) memsets x and hence
new_p); always detects 1/ if freed via free_acquired() and/or freeproc()
(very likely, otherwise memory may be leaked); probably detects 1/ even
if freed directly via free() (because the canary is the first member of
proc_t, likely to be overwritten by free()); but can not detect 1/ if
free() does not write to new_p's chunk at all.
Moreover, accessing new_p->tid to check the canary in case 1/ is itself
a use-after-free, so a better long-term solution should be implemented
at some point (we wanted to avoid intrusive and backward-incompatible
changes in this library function, hence this imperfect workaround).
proc/readproc.c: Prevent integer overflows in readproctab*().
If an integer overflow is about to be reached, call xalloc_err_handler()
(since it would have been caught by calloc() or reallocarray()) and then
exit(): these integer overflows are far from reachable, with the current
PID_MAX_LIMIT (2^22), so if they are there is something very wrong going
on. Note: we check the n_*alloc variables against INT_MAX even when they
are size_t because they are later stored as int in a struct proc_data_t.
projects / procps-ng / log