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.
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:
$ pmap -x -A 6FFF00000000,7FFF00000000 $$
...
Address Kbytes RSS Dirty Mode Mapping
000055b3d1e9b000 0 912 0 r-xp (null)
000055b3d2194000 0 16 16 r--p (null)
000055b3d2198000 0 36 36 rw-p (null)
...
Removed const as this causes problems elsewhere.
Signed-off-by: Craig Small <csmall@enc.com.au>
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.
Signed-off-by: Craig Small <csmall@enc.com.au>
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.
Signed-off-by: Craig Small <csmall@enc.com.au>
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.
Signed-off-by: Craig Small <csmall@enc.com.au>
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".
Signed-off-by: Craig Small <csmall@enc.com.au>
Also, simplify the code slightly (but functionally equivalent). Check
the return value of xstrdup() only once (yes, it can return NULL).
Adapted slightly to remove goto and leave the format of checks the same.
A lot of the fixes were already in newlib, caught by coverity
References:
commit 25f655891f
Signed-off-by: Craig Small <csmall@enc.com.au>
First problem: saved_argc was used to calculate the size of the array,
but saved_argc was never initialized. This triggers an immediate heap-
based buffer overflow:
$ skill -c0 -c0 -c0 -c0
Segmentation fault (core dumped)
Second problem: saved_argc was not the upper bound anyway, because one
argument can ENLIST() several times (for example, in parse_namespaces())
and overflow the array as well.
Third problem: integer overflow of the size of the array.
No need to "pid_count++;" because "ENLIST(pid," does it already. Right
now this can trigger a heap-based buffer overflow.
Also, remove the unneeded "pid_count = 0;" (it is static, and
skillsnice_parse() is called only once; and the other *_count variables
are not initialized explicitly either).
The memmove() itself does not move the NULL-terminator, because nargs is
decremented first. Copy how skill_sig_option() does it: decrement nargs
last, and remove the "if (nargs - i)" (we are in "while (i < nargs)").
man getline: "If *lineptr is set to NULL and *n is set 0 before the
call, then getline() will allocate a buffer for storing the line. This
buffer should be freed by the user program even if getline() failed."
Note: unlike "size" and "omit_size", "path_alloc_size" is not multiplied
by "sizeof(struct el)" but the checks in grow_size() allow for a roughly
100MB path_alloc_size, which should be more than enough for readlink().
Do it explicitly instead of the implicit "longjmp() cannot cause 0 to be
returned. If longjmp() is invoked with a second argument of 0, 1 will be
returned instead."
Otherwise (for example), if the (undocumented) opt_echo is set, but not
opt_long, and not opt_longlong, and not opt_pattern, there is a call to
xstrdup(cmdoutput) but cmdoutput was never initialized:
sleep 60 & echo "$!" > pidfile
env -i LD_DEBUG=`perl -e 'print "A" x 131000'` pkill -e -c -F pidfile | xxd
...
000001c0: 4141 4141 4141 4141 4141 4141 4141 4141 AAAAAAAAAAAAAAAA
000001d0: 4141 4141 4141 4141 fcd4 e6bd e47f 206b AAAAAAAA...... k
000001e0: 696c 6c65 6420 2870 6964 2031 3230 3931 illed (pid 12091
000001f0: 290a 310a ).1.
[1]+ Terminated sleep 60
(the LD_DEBUG is just a trick to fill the initial stack with non-null
bytes, to show that there is uninitialized data from the stack in the
output; here, an address "fcd4 e6bd e47f")
Not exploitable (not under an attacker's control), but still a potential
non-security problem. Copied, fixed, and used the grow_size() macro from
pidof.c.
Signed-off-by: Craig Small <csmall@enc.com.au>
We now use the actual terminfo 'max_colors' value with
the 'color mapping' screen, not that hard coded '256'.
Reference(s):
https://gitlab.com/procps-ng/procps/issues/96
Signed-off-by: Jim Warner <james.warner@comcast.net>
When not displaying all tasks (the 'i' toggle is off),
the concept of vertical scrolling has no real meaning.
However, only 2 keys (up/down) impacting that vertical
position were currently being disabled with this mode.
This patch will extend such treatment to the following
additional vertical impact keys: pgup,pgdn,home & end.
Signed-off-by: Jim Warner <james.warner@comcast.net>
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>
Unlikely to ever happen, since it would imply a very large string, but
better safe than sorry.
---------------------------- adapted for newlib branch
. now uses 'xmalloc' vs. unchecked stdlib 'malloc'
. the member 'need' was removed from 'format_node'
Signed-off-by: Jim Warner <james.warner@comcast.net>
To avoid an out-of-bounds access at checkoff[tmp]. The strspn() at the
beginning of the function protects against it already, but double-check
this in case of some future change.
Right now, "we _exit() anyway" is not always true: for example, the
default action for SIGURG is to ignore the signal, which means that
"kill(getpid(), signo);" does not terminate the process. Call _exit()
explicitly, in this case (rather than exit(), because the terminating
kill() calls do not call the functions registered with atexit() either).
Before "strlen(outbuf)", if one of the pr_*() functions forgot to do it.
This prevents an out-of-bounds read in strlen(), and an out-of-bounds
write in "outbuf[sz] = '\n'". Another solution would be to replace
strlen() with strnlen(), but this is not used anywhere else in the
code-base and may not exist in all libc's.
---------------------------- adapted for newlib branch
. adapted via 'patch' without rejections
Signed-off-by: Jim Warner <james.warner@comcast.net>
pr_bsdstart(): Replace "strcpy(outbuf," with "snprintf(outbuf, COLWID,"
(which is used in all surrounding functions). (side note: the fact that
many pr_*() functions simply return "snprintf(outbuf, COLWID," justifies
the "amount" checks added to show_one_proc() by the "ps/output.c:
Replace strcpy() with snprintf() in show_one_proc()." patch)
pr_stime(): Check the return value of strftime() (in case of an error,
"the contents of the array are undefined").
help_pr_sig(): Handle the "len < 8" case, otherwise "sig+len-8" may
point outside the sig string.
pr_context(): Handle the empty string case, or else "outbuf[len-1]"
points outside outbuf.
---------------------------- adapted for newlib branch
. logic is quite different with 'stacks' vs. 'proc_t'
Signed-off-by: Jim Warner <james.warner@comcast.net>
Enforce a maximum max_rightward of OUTBUF_SIZE-1, because it is used in
constructs such as "snprintf(outbuf, max_rightward+1," (we could remove
the extra check at the beginning of forest_helper() now, but we decided
to leave it, as a precaution and reminder).
The minimum max_rightward check is not strictly needed, because it is
unsigned. However, we decided to add it anyway:
- most of the other variables are signed;
- make it visually clear that this case is properly handled;
- ideally, the minimum max_rightward should be 1, not 0 (to prevent
integer overflows such as "max_rightward-1"), but this might change
the behavior/output of ps, so we decided against it, for now.
Instead, we fixed the only function that overflows if max_rightward is
0. Also, enforce the same safe range for max_leftward, although it is
never used throughout the code-base.
---------------------------- adapted for newlib branch
. adapted via 'patch' without rejections
Signed-off-by: Jim Warner <james.warner@comcast.net>
This strcpy() should normally not overflow outbuf, but names can be
overridden (via -o). Also, check "amount" in all cases.
---------------------------- adapted for newlib branch
. we don't use that 'likely/unlikely' crap in newlib
Signed-off-by: Jim Warner <james.warner@comcast.net>
It is static and not used anywhere.
---------------------------- adapted for newlib branch
. limited to whitespace/formatting differences
Signed-off-by: Jim Warner <james.warner@comcast.net>
We decided not to check the return value of the mprotect() calls,
because they are not vital to the operation of ps.
---------------------------- adapted for newlib branch
. many formatting/whitespace differences
Signed-off-by: Jim Warner <james.warner@comcast.net>
