All TIC delta fields are checked for possible negative
results and set to zero when found. This is done so as
to protect against potential anomalies which depend on
kernel version and/or toggling cpus offline or online.
[ it's probably unnecessary with the latest kernels, ]
[ except for iowait. documentation suggests it might ]
[ decrease which would then create a negative delta. ]
The same approach is employed for most of the 'system'
deltas (ctxt, intr & procs_created). However, with two
of the fields (procs_blocked & procs_running) negative
results were allowed. But it now seems such a division
is unwise so this patch will allow all to go negative.
[ rather than force any 'system' delta value to show ]
[ what's logical, we'll now let all reflect reality. ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch just standardizes/normalizes the whitespace
employed within a couple of nearly identical #defines.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Under newlib, the only caller of the readproc routines
is that pids module. And in every case, the address of
some static proc_t structure has always been provided.
As a result, there is no need for the logic supporting
calloc() for a possible NULL pointer which was present
in both of those readproc() and readeither() routines.
Additionally, that pids module takes ownership of most
dynamically acquired 'str' plus 'strv' memory whenever
assigning to a results structure. So, henceforth under
the free_acquired() guy we will only free those string
fields which might exist when not explicitly selected.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch just eliminates a parameter present for the
simple_readtask() function which is not needed nor has
it ever actually been used. It will make calls to that
function (via taskreader ptr) slightly more efficient.
Signed-off-by: Jim Warner <james.warner@comcast.net>
When some cleanup was performed on the readproc module
in the commit shown below, some residual code involved
with the 'did_fake' flag remained. Since such logic no
longer served any real need, this patch will whack it.
Reference(s):
. cleanup of readproc functions
commit 887bb51016
Signed-off-by: Jim Warner <james.warner@comcast.net>
No libprocps user expects signal values to be returned
as 'long long' quantities. More importantly the <PIDS>
api only returns a 'str' result for signal categories.
So this patch eliminates all the conditional code that
depends on the absence of the #define 'SIGNAL_STRING'.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch will just correct an oops introduced in the
commit shown below. Thank goodness both 'str' & 'strv'
occupy the same storage location in that result union.
Reference(s):
. standardize 'errno' management
commit 06be33b43e
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch just implements an equivalent to the master
branch 'CPU_ZEROTICS' provision. However, the original
impetus for that earlier implementation was ultimately
attributed to a likely kernel anomaly since corrected.
As a result, in this newlib implementation we take the
opposite approach to the default behavior. There is no
adjustment to TIC_SUM_DELTA values if fewer ticks than
expected are recorded, unless the define is activated.
The commit shown below explains why the 'CPU_ZEROTICS'
define was retained in spite of the fix to the kernel.
Reference(s):
. issue referencing CPU_ZEROTICS
https://gitlab.com/procps-ng/procps/issues/132
. master branch CPU_ZEROTICS summary
commit ee3ed4b45e
. lengthy thread leading to CPU_ZEROTICS
https://www.freelists.org/post/procps/CStates-handling-new-switch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Some parts of our newlib implementation are the result
of functions which have been propagated from module to
module. In particular, those 'cleanup_stacks' routines
are all similar & likely originated in the <pids> api.
In that interface there was a need to free dynamically
acquired memory before the result structure was reused
to satisfy subsequent 'get', 'select' or 'reap' calls.
This, in turn, led to a concept of 'dirty' stacks with
the need to call one of two 'cleanup_stack' functions.
None of the remaining interfaces deal with such memory
yet they each had their own 'cleanup_stack' functions.
Those functions were responsible for resetting each of
the result unions to zero, excluding any 'noop' items.
The bottom line is that for all interfaces, repetitive
calls would require iterating through the stack(s) two
separate times: once to 'cleanup' another to 'assign'.
With this commit we will reduce iterations to just the
'assign' routine. A reset to zero will be accomplished
in the 'extra' item set routine (which is the only one
actually requiring any reset). All other items will be
reinitialized automatically by a new current set value
or upon reallocation when an items compliment changes.
In the <pids> interface, any freeing of dynamic memory
could have been accomplished by adding that 'freefunc'
check to the 'assign' function. However, that requires
an Item_table test with every item. Instead, we'll now
satisfy such needs as the very first step in those set
functions responsible for dynamically acquired memory.
[ the <pids> api retains 2 'cleanup_stack' functions ]
[ to accommodate stack(s) 'reset' & to serve 'unref' ]
Lastly, all the 'itemize_stack' functions were tweaked
by eliminating an unnecessary initialization of result
unions. That objective was already accomplished by the
calloc() in a 'stacks_alloc' function or the remaining
'cleanup_stack' routine found in the <pids> interface.
Signed-off-by: Jim Warner <james.warner@comcast.net>
These changes are an outgrowth of the research/testing
behind the previous commit. There is no commingling of
select/reap stacks in interfaces beyond the <pids> api
since there's no need to support any 'reset' function.
However, those <pids> changes prompted a review of all
interfaces offering that 'stacks_fetch' function, thus
revealing 2 instances of useless logic/wasted efforts.
Signed-off-by: Jim Warner <james.warner@comcast.net>
When the <pids> api was refactored in the commit shown
below, one objective was enabling the simultaneous use
of 'get' & 'select/reap' functions. Unlike other 'get'
functions, this <pids> 'get' acts as an iterator where
successive calls will return successive tasks/threads.
However, that goal wasn't quite met since a stack used
by 'get' was commingled with the 'select/reap' stacks.
Such commingling supported the 'reset' function, again
a provision which was unique to this <pids> interface.
Unfortunately, some poor assumptions in 'stacks_fetch'
produced a SEGV whenever 'reap/select' followed 'get'.
Thus, this patch addresses those issues and guarantees
such commingled stacks (extents) will be accommodated.
Reference(s):
. standardize portions of interface, <PIDS> api
commit 9ebadc1438
Signed-off-by: Jim Warner <james.warner@comcast.net>
The commit referenced below addressed (some) anomalies
surrounding 'strv' pointers. However, there remained a
couple quirks involving a potential NULL return value.
Any NULL values returned from the old library readproc
guys would cause no real harm for newlib. But they did
produce the misleading "[ duplicate ENUM_ID ]" result.
The following all represent potential NULL results and
suggest shortcomings in testing of that earlier patch.
. kernel threads do not have cgroup, cmdline & environ
. even if present environ could require root to access
So, this patch reverts a portion of the earlier commit
and ensures when some vectored string is not available
a traditional dash ('-') is the 'strv' returned value.
[ and we'll also correct one typo in the header file ]
Reference(s):
. eliminated a final potential NULL, <PIDS> api
commit 09503dc597
Signed-off-by: Jim Warner <james.warner@comcast.net>
This commit is intended as a refinement of the patches
mentioned below, where origins/sources of newlib items
were added to the header files for user documentation.
However, if those additions are to be truly effective,
along with kernel documentation (where available), the
following prerequisites must also have been satisfied:
. our identifiers closely align with linux field names
. our derived items are documented or self-documenting
Satisfying those prerequisites prompted this patch and
for these changes, kernel sources were emphasized over
available documentation (shame on me, it should always
have been so). And, while some 'new' fields were found
to be conditional, they were included unconditionally.
These changes appear more extensive than they actually
need be since I have attempted to enforce some spacing
conventions. So, I've summarize the significant things
in the sections that follow. For a proper perspective,
use: 'git diff --ignore-space-change' (good as alias).
___________________________________________ <PIDS> api
This api is unique in that there exists many different
file/directory origins subordinate to /proc/<pid>. And
our item identifiers are sometimes coerced so as to be
able to group related or similar enumerators together.
So, users needed more help relating our identifiers to
an actual documented field. Thus, we will now also add
the field names as with 'stat: delayacct_blkio_ticks'.
Each item ending with a '_C' now consistently includes
both the parent's count/time plus waited for children.
That 'RTPRIO' guy was renamed/relocated as PRIORITY_RT
since its original name is an implementation artifact.
___________________________________________ <STAT> api
The only api change was to correct a typo ('dervied').
_________________________________________ <VMSTAT> api
Even ignoring white space, this interface received the
largest number of changes. Mostly, this was because of
deficiencies in the proc(5) documentation. Recall that
this documentation already sorely lacks any substance.
Usually, just kernel releases are noted, not contents.
When compared to kernel source, that proc(5) contained
many non-existent fields and also omitted many others.
________________________________________ <MEMINFO> api
Sadly, with this api many of the changes were simply a
correction of some earlier 'human error' where several
fields where hashed then tracked but never represented
with an item enumerator in this meminfo.h header file.
_______________________________________ <SLABINFO> api
The 'SLABS' (summary) & 'SLABNODE' items were reversed
since the former are derived from the separate caches.
More significantly, those 'SLABNODE' guys were renamed
to 'SLAB' since they concern individual caches and the
concept of 'nodes' is really an implementation detail.
Also, several enumerators were changed to more closely
agree with official slabinfo(5) documentation referred
to in what we're treating as a base document: proc(5).
Lastly, while those 'SLABS' items are solely a product
of our library and not represented in slabinfo(5), the
names attempt to parallel those found as 'SLAB' items.
______________________________________ <DISKSTATS> api
One enumeration identifier was changed so as to better
reflect its relationship to that actual documentation:
'Documentation/iostats.txt', as referenced in proc(5).
Reference(s):
. 12/2018, item origins added (and commit msg history)
commit 96d59cbf46
. 01/2019, <stat> origins tweaked
commit 201e816b26
Signed-off-by: Jim Warner <james.warner@comcast.net>
Rather than offer three separate patches, they've been
consolidated in this single commit. All are related in
that they surfaced while preparing a subsequent patch.
------------------------------------------------------
library: correct a broken '#if define', <SLABINFO> api
It was introduced (embarrassingly) in the patch below.
Reference(s):
commit 97d078a9af
------------------------------------------------------
library: correct a broken 'GET' macro, <DISKSTATS> api
In the patch referenced below, which purported to make
all the 'GET' macros robust, the 'DISKSTATS_GET' macro
was broken. A necessary parameter wasn't passed to the
subsequently invoked function: procps_diskstats_get().
Reference(s):
commit bef8c7fb70
------------------------------------------------------
library: correct a broken 'sort' func, <DISKSTATS> api
In the commit shown below, an attempt to normalize the
errno handling, the sort function inadvertently lost 1
crucial line of code which produces a consistent SEGV.
Reference(s):
commit 06be33b43e
Signed-off-by: Jim Warner <james.warner@comcast.net>
Since the patch referenced below traded a compile-time
'sizeof' directive for a run-time 'strlen' call, there
is no need to declare lxc patterns as explicit arrays.
We'll also use the actual lxc patterns by omitting the
beginning slashes ('/') for both of those definitions.
And, looking to the future when most/all lxc users are
using the most recent lxc release, we will make things
slightly more efficient by reversing those two pattern
literals so the most recent pattern was checked first.
Of course, such a change only benefits tasks which are
running in a container. For the majority of processes,
both literals will be compared in that 'if' statement,
assuming the 'LXC' field is currently being displayed.
[ plus, a leftover parenthesis pair has been removed ]
Reference(s):
commit 288d759b8b
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch just polishes the 'origin' comments for the
<STAT> header file. In particular those derived/unique
items (the 'SUM' guys) will now be properly explained.
[ in order to employ the 'derived from above' phrase ]
[ with their DELTA versions, all SUM items had to be ]
[ relocated (and some renamed). in turn, that had an ]
[ impact on many portions of the .c source file too. ]
Reference(s):
. summary calculations introduced
commit 2c86c4984a
. origins added to header files
commit 96d59cbf46
Signed-off-by: Jim Warner <james.warner@comcast.net>
This small change is an outgrowth of the research into
the bug represented by that merge request shown below.
With the master branch, a real buglet was subsequently
addressed. In this newlib branch, no bug existed since
the <stat> API relies solely on just cpus reflected in
(and parsed from) the kernel's /proc/stat pseudo file.
[ since that procps_stat_new() priming read about to ]
[ be performed will value info->cpus.total, there is ]
[ no need to separately invoke a procps_cpu_count(). ]
Reference(s):
https://gitlab.com/procps-ng/procps/merge_requests/82
Signed-off-by: Jim Warner <james.warner@comcast.net>
The merge request shown below prompted (thankfully) an
examination of our lxc containers logic in readproc.c.
As it turns out, the lxc folks changed that eyecatcher
used to identify containers within a task cgroup file.
So this patch, with little extra cost, will enable the
libprocps lxc_containers() guy to handle both strings.
[ additionally, I was shocked to find lxc allows the ]
[ eyecatcher to be changed at ./configure time. such ]
[ a provision has always existed. unfortunately, the ]
[ changed value was only available to root, assuming ]
[ one wished to tackle that undocumented liblxc api. ]
Reference(s):
. what prompted lxc support reevaluation
https://gitlab.com/procps-ng/procps/merge_requests/82
. original lxc support introduced
commit 0557504f9c
Signed-off-by: Jim Warner <james.warner@comcast.net>
A lack of documentation seems to be the major obstacle
to releasing this new library. So, in an effort to get
the ball rolling again, this patch adds the origins of
each item as a comment to six of the new header files.
However, before reviewing how such changes may benefit
that documentation objective, it seemed appropriate to
first reflect on newlib's background & current status.
___________________________________________ BACKGROUND
Discussions about and work on a new library began back
in July 2012 but quickly died. After a lull of 2 years
those discussions were resumed in August 2014 but soon
died also (and no code survived the gitorious demise).
With those early discussions, the recommended approach
was to encapsulate all of the libprocps data offerings
in individual functions. When it came to extensibility
it was suggested we should rely on symbols versioning.
Unfortunately that approach would have made for a huge
Application Programming Interface virtually impossible
to master or even document. And, runtime call overhead
would have been substantial for ps and especially top.
So, an alternative design was sought but there were no
new suggestions/contributions via freelists or gitlab.
Thus, in spite of a lack of library design experience,
the procps-ng team (Craig & Jim) set out to develop an
alternative API, more concise and with lower overhead.
Reference(s):
. 07/01/2012, begin library design discussion
https://www.freelists.org/post/procps/Old-library-calls
. 08/12/2014, revival of library design discussion
https://www.freelists.org/post/procps/libprocs-redesign
_____________________________________ DESIGN EVOLUTION
Our newlib branch first appeared on June 14, 2015. And
our current API actually represents the 4th generation
during the past 3 years of evolution. First, there was
a basic 'new', 'get' and 'unref' approach, using enums
to minimize the proliferation of 'get' function calls.
Then, in anticipation of other programs like ps, where
multiple fields times multiple processes would greatly
increase the number of 'get' function calls, a concept
of 'chains' was introduced. This became generation #2.
Such 'chains' proved unnecessarily complex so 'stacks'
replaced them. This was considered the 3rd generation,
but too many implementation details were still exposed
requiring those users to 'alloc', 'read', 'fill', etc.
Finally, a 4th generation emerged representing several
refinements to standardize and minimize those exported
functions, thus hiding all implementation details from
the users. Lastly, handling of 'errno' was normalized.
Reference(s):
. 06/14/2015, revival of new API discussion
https://www.freelists.org/post/procps/The-library-API-again
. 06/24/2015, birth of the newlib branch
https://www.freelists.org/post/procps/new-library
. 06/29/2015, 2nd generation introduced 'chains'
https://www.freelists.org/post/procps/new-library,8
. 07/22/2015, 3rd generation introduced 'stacks'
https://www.freelists.org/post/procps/newlib-stacks-vs-chains
. 06/18/2016, 4th generation refinements begin
https://www.freelists.org/post/procps/newlib-generation-35
. 11/10/2017, 4th generation standardized 'errno'
https://www.freelists.org/post/procps/some-more-master-newlib-stuff
_______________________________________ CURRENT DESIGN
Central to this new design is a simple 'result' struct
reflecting an item plus its value (thanks to a union).
As a user option, these item structures can be grouped
into 'stacks', yielding many results with just 1 call.
Such a 'stack' can be seen as a variable length record
whose content/order is determined solely by the users.
Within that 'result' structure, the union has standard
C language types so there is never a doubt how a value
should be used in a printf statement. Given that linux
requires a least a 32-bit platform the only difference
in capacity surrounds 'long' integers. And, where such
types might be used, the 32-bit maximums are adequate.
The items themselves are simply enumerators defined in
the respective header files. A user can name any items
of interest then the library magically provides result
structure(s). The approach was proven to be extensible
without breaking the ABI (in commit referenced below).
The 6 major APIs each provide for the following calls:
. 'new' ---------> always required as the first call .
. 'ref' -------------------------> strictly optional .
. 'unref' --------> optional, if ill-behaved program .
. 'get' --------------------> retrieve a single item .
. 'select' ----------------> retrieve multiple items .
And the 'get' and 'select' functions provide for delta
results representing the difference between successive
get/select calls (or a 'new' then 'get/select' call).
For the <diskstats>, <pids>, <slabinfo> & <stat> APIs,
where results are unpredictable, a 'reap' function can
return multiple result structures for multiple stacks.
The <pids> API differs from others in that those items
of interest must be provided at 'new' or 'reset' time,
a function unique to this API. And the <pids> 'select'
function requires PIDs or UIDs which are to be fetched
which then operates as a subset of 'reap'. Lastly, the
'get' function is an iterator for successive PIDs/TIDs
returning items previously identified via 'new/reset'.
To provide assistance to users during development, the
special header 'proc/xtra-procps-debug.h' is available
to check type usage against library expectations. That
check is activated by including this header explicitly
or via build using: ./configure '-DXTRA_PROCPS_DEBUG'.
Reference(s):
. 08/05/2016, type validation introduced
https://www.freelists.org/post/procps/newlib-types-validation
commit e3270d463d
. 08/11/2016, extensibility while preserving ABI example
https://www.freelists.org/post/procps/new-meminfo-fields
commit 09e1886c9e
_________________________ INITIAL DOCUMENTATION EFFORT
The initial attempt, referenced below, dealt primarily
with the <pids> interface. Separate man pages for each
exported function were created. Plus there was another
document describing the items, among other miscellany.
Adopting such an approach encounters several problems:
1. In order to use these man pages, users are required
to already know how to use the library. Or alternately
one could randomly search each of them while trying to
ascertain which function call satisfies their need and
what exactly was the proper compliment/order required.
2. While we can explain what all of those <pids> items
represent, that certainly isn't true for all the APIs.
See the gaps in kernel documentation for <meminfo> and
complete lack of documentation with that <vmstat> API.
3. Our documentation effort should take pains to avoid
unnecessary implementation details. Here's an example:
. "The pointer to info will have memory"
. "allocated and a structure created."
Alternatively, the following conveys user requirements
while not offering any internal implementation detail:
. "You must provide the address of a NULL"
. "info structure pointer."
Reference(s):
. 01/04/2017, initial documentation offering
https://www.freelists.org/post/procps/Using-reap-and-get
commit 2598e9f2ce
___________________ RECOMMENDED DOCUMENTATION APPROACH
I recommend that the newlib documentation consist of 3
man pages only. The first would cover the 5 major APIs
and their common functions. The second would deal with
the <pids> API exclusively, explaining how it differs.
Any remaining exported libproc functions which are yet
to be included could be represented in a 3rd document.
For these new documents the following are are assumed:
1. Since we will not be able to document all items, we
shouldn't try to document any items. We should instead
rely on proc(5) or Documentation/filesystems/proc.txt.
2. Program development often involves referencing some
header file(s). So, make that an absolute requirement.
3. With the addition of item origins, represented with
this commit, and considering that 'types' were already
present, the header file might be all some users need.
4. And who knows, when a user of our libproc complains
about gaps in their documentation, it might prompt the
kernel folks to correct those long standing omissions.
To summarize, I suggest that we replace that libproc.3
document with a more general one explaining the basics
of accessing this new library and the common calls for
most of the major interfaces. We can then create a new
document (libproc-pids.3?), which explains differences
in using the <PIDS> application programming interface.
A final document (libproc-misc.3?) covers what's left.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This change is being made in anticipation of adding the
source origin of each item to the <pids.h> header file.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch will bring the <meminfo> API into line with
that proc(5) document. There were several undocumented
fields that were not noted and these two were omitted:
. 'MmapCopy' was conditional on the #define CONFIG_MMU
. 'Quicklists' depends on the #define CONFIG_QUICKLIST
And we're about to get the following new field in 4.20
which will be represented, at least, in that proc.txt:
. 'KReclaimable' will include SReclaimable plus others
Signed-off-by: Jim Warner <james.warner@comcast.net>
If we ever were to eliminate the procps.h header file,
as discussed in the thread referenced below, then that
would impair the current XTRA_PROCPS_DEBUG provisions.
The only remaining way to verify result types would be
to explicitly include that <proc/xtra-procps-debug.h>.
So, this commit will once again enable the ./configure
provision for defining the -DXTRA_PROCPS_DEBUG option.
Reference(s):
https://www.freelists.org/post/procps/newlib-Qualys-patches,6
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch is the first of three implementing a newlib
branch version of that Jan Rybar master merge request.
With this series we'll ultimately extend 'EXE' support
to both ps and top (plus, everyone else who wants it).
Reference(s):
. master branch merge request
https://gitlab.com/procps-ng/procps/merge_requests/66
Signed-off-by: Jim Warner <james.warner@comcast.net>
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>
This refactor was done in response to the Qualys patch
referenced below, which deals with some 'readeither()'
flaws under the master branch. Under our newlib branch
those flaws mostly disappear since the function is now
private. But without a redesign the #define is broken.
When the #define FALSE_THREADS is active, some special
strings showing "[ duplicate ENUM ]" will appear under
each child thread. Note that the real reason for those
appearing isn't being exercised, only their mechanics.
In reality, they only show when a user duplicates such
enums in a results stack & only 1 instance can own it.
Reference(s):
. original qualys patch
0084-proc-readproc.c-Work-around-a-design-flaw-in-readeit.patch
. QUICK_THREADS became FALSE_THREADS
commit c546d9dd44
Signed-off-by: Jim Warner <james.warner@comcast.net>
In the new library 'cmd' is dynamically allocated just
like 'cmdline'. This will align us with the ref below.
Reference(s):
. master branch increase to 64
commit 2cfdbbe897
Signed-off-by: Jim Warner <james.warner@comcast.net>
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).
---------------------------- adapted for newlib branch
. adapted via 'patch' (rejected due to 'xcalloc' ref)
. with loss of both readproctab functions, most no longer true
Signed-off-by: Jim Warner <james.warner@comcast.net>
If QUICK_THREADS is not defined (it is not by default, but most
distributions enable it) and task_dir_missing is true (only on very old
kernels), then readtask() forgets to reset some of the struct proc_t t's
members, which later results in double-free()s in free_acquired().
For now, we simply synchronized the list of members to be reset with the
list of members freed in free_acquired().
---------------------------- adapted for newlib branch
. now 'cmd' is also dynamic
. just synchronized with those freed in free_acquired
. QUICK_THREADS is now FALSE_THREADS, serving different purpose
. entire patch will be effectively reverted with upcoming refactor
Signed-off-by: Jim Warner <james.warner@comcast.net>
Replace xmalloc() with xcalloc().
---------------------------- adapted for newlib branch
. trade xcalloc() for calloc()
. thus we must account for potential ENOMEM
Signed-off-by: Jim Warner <james.warner@comcast.net>
Replace memcpy+strcpy with snprintf.
---------------------------- adapted for newlib branch
. adapted via 'patch' (without rejections)
Signed-off-by: Jim Warner <james.warner@comcast.net>
Check the return value of snprintf(), otherwise dst may point
out-of-bounds when it reaches the end of the dst_buffer (the snprintf()
always returns 1 in that case, even if there is not enough space left),
and vMAX becomes negative and is passed to snprintf() as a size_t.
---------------------------- adapted for newlib branch
. adapted via 'patch (without rejections)
Signed-off-by: Jim Warner <james.warner@comcast.net>
This detects an integer overflow of "strlen + 1", prevents an integer
overflow of "tot + adj + (2 * pSZ)", and avoids calling snprintf with a
string longer than INT_MAX. Truncate rather than fail, since the callers
do not expect a failure of this function.
---------------------------- adapted for newlib branch
. logic is now in pids.c
. former 'vectorize_this_str' is now 'pids_vectorize_this'
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Prevent an out-of-bounds write if sz is 0.
2/ Limit sz to INT_MAX, because the return value is an int, not an
unsigned int (and because if INT_MAX is equal to SSIZE_MAX, man 2 read
says "If count is greater than SSIZE_MAX, the result is unspecified.")
3/ Always null-terminate dst (unless sz is 0), because a return value of
0 because of an open() error (for example) is indistinguishable from a
return value of 0 because of an empty file.
4/ Use an unsigned int for i (just like n), not an int.
5/ Check for snprintf() truncation.
---------------------------- adapted for newlib branch
. adapted via 'patch (without rejections)
Signed-off-by: Jim Warner <james.warner@comcast.net>
Note: this is by far the most important and complex patch of the whole
series, please review it carefully; thank you very much!
For this patch, we decided to keep the original function's design and
skeleton, to avoid regressions and behavior changes, while fixing the
various bugs and overflows. And like the "Harden file2str()" patch, this
patch does not fail when about to overflow, but truncates instead: there
is information available about this process, so return it to the caller;
also, we used INT_MAX as a limit, but a lower limit could be used.
The easy changes:
- Replace sprintf() with snprintf() (and check for truncation).
- Replace "if (n == 0 && rbuf == 0)" with "if (n <= 0 && tot <= 0)" and
do break instead of return: it simplifies the code (only one place to
handle errors), and also guarantees that in the while loop either n or
tot is > 0 (or both), even if n is reset to 0 when about to overflow.
- Remove the "if (n < 0)" block in the while loop: it is (and was) dead
code, since we enter the while loop only if n >= 0.
- Rewrite the missing-null-terminator detection: in the original
function, if the size of the file is a multiple of 2047, a null-
terminator is appended even if the file is already null-terminated.
- Replace "if (n <= 0 && !end_of_file)" with "if (n < 0 || tot <= 0)":
originally, it was equivalent to "if (n < 0)", but we added "tot <= 0"
to handle the first break of the while loop, and to guarantee that in
the rest of the function tot is > 0.
- Double-force ("belt and suspenders") the null-termination of rbuf:
this is (and was) essential to the correctness of the function.
- Replace the final "while" loop with a "for" loop that behaves just
like the preceding "for" loop: in the original function, this would
lead to unexpected results (for example, if rbuf is |\0|A|\0|, this
would return the array {"",NULL} but should return {"","A",NULL}; and
if rbuf is |A|\0|B| (should never happen because rbuf should be null-
terminated), this would make room for two pointers in ret, but would
write three pointers to ret).
The hard changes:
- Prevent the integer overflow of tot in the while loop, but unlike
file2str(), file2strvec() cannot let tot grow until it almost reaches
INT_MAX, because it needs more space for the pointers: this is why we
introduced ARG_LEN, which also guarantees that we can add "align" and
a few sizeof(char*)s to tot without overflowing.
- Prevent the integer overflow of "tot + c + align": when INT_MAX is
(almost) reached, we write the maximal safe amount of pointers to ret
(ARG_LEN guarantees that there is always space for *ret = rbuf and the
NULL terminator).
---------------------------- adapted for newlib branch
. there were many formatting differences
. i introduced several myself (especially comments)
. stdlib 'realloc' used, not that home grown xrealloc
. stdlib 'realloc' required extra 'return NULL' statement
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Replace sprintf() with snprintf() (and check for truncation).
2/ Prevent an integer overflow of ub->siz. The "tot_read--" is needed to
avoid an off-by-one overflow in "ub->buf[tot_read] = '\0'". It is safe
to decrement tot_read here, because we know that tot_read is equal to
ub->siz (and ub->siz is very large).
We believe that truncation is a better option than failure (implementing
failure instead should be as easy as replacing the "tot_read--" with
"tot_read = 0").
---------------------------- adapted for newlib branch
. no real changes, patch refused due to mem alloc & failure return
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Use a "size_t num" instead of an "unsigned num" (also, do not store
the return value of sscanf() into num, it was unused anyway).
2/ Check the return value of strchr() and strrchr().
3/ Never jump over the terminating null byte with "S = tmp + 2".
---------------------------- adapted for newlib branch
. newlib doesn't use that 'unlikely' crap
. the cmd field is now also dynamic (like cmdline)
. thus we must account for potential ENOMEM
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Prevent an integer overflow of t.
2/ Avoid an infinite loop if s contains characters other than comma,
spaces, +, -, and digits.
3/ Handle all possible return values of snprintf().
---------------------------- adapted for newlib branch
. we can't use xrealloc(), so we use realloc() instead
. and must account for a mem failure via a return of 1
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Do not read past the terminating null byte when hashing the name.
2/ S[x] is used as an index, but S is "char *S" (signed) and hence may
index the array out-of-bounds. Bit-mask S[x] with 127 (the array has 128
entries).
3/ Use a size_t for j, not an int (strlen() returns a size_t).
Notes:
- These are (mostly) theoretical problems, because the contents of
/proc/PID/status are (mostly) trusted.
- The "name" member of the status_table_struct has 8 bytes, and
"RssShmem" occupies exactly 8 bytes, which means that "name" is not
null-terminated. This is fine right now, because status2proc() uses
memcmp(), not strcmp(), but it is worth mentioning.
---------------------------- adapted for newlib branch
. newlib doesn't use that 'unlikely' crap
. newlib also had a '#ifdef FALSE_THREADS'
Signed-off-by: Jim Warner <james.warner@comcast.net>
This function is unused (SIGNAL_STRING is defined by default, and if it
is not, procps does not compile -- for example, there is no "outbuf" in
help_pr_sig()) but fix it anyway. There are two bugs:
- it accepts non-hexadecimal characters (anything >= 0x30);
- "(c - (c>0x57) ? 0x57 : 0x30)" is always equal to 0x57.
---------------------------- adapted for newlib branch
. newlib doesn't use that 'unlikely' crap
Signed-off-by: Jim Warner <james.warner@comcast.net>
This solves several problems:
1/ outbuf[1] was written to, but not outbuf[0], which was left
uninitialized (well, SECURE_ESCAPE_ARGS() already fixes this, but do it
explicitly as well); we know it is safe to write one byte to outbuf,
because SECURE_ESCAPE_ARGS() guarantees it.
2/ If bytes was 1, the write to outbuf[1] was an off-by-one overflow.
3/ Do not call escape_str() with a 0 bufsize if bytes == overhead.
4/ Prevent various buffer overflows if bytes <= overhead.
Simply rearrange the old comparisons. The new comparisons are safe,
because we know from previous checks that:
1/ wlen > 0
2/ my_cells < *maxcells (also: my_cells >= 0 and *maxcells > 0)
3/ len > 1
4/ my_bytes+1 < bufsize (also: my_bytes >= 0 and bufsize > 0)
This should never happen, because wcwidth() is called only if iswprint()
returns nonzero. But belt-and-suspenders, and make it visually clear
(very important for the next patch).
The SECURE_ESCAPE_ARGS() macro solves several potential problems
(although we found no problematic calls to the escape*() functions in
procps's code-base, but had to thoroughly review every call; and this is
library code):
1/ off-by-one overflows if the size of the destination buffer is 0;
2/ buffer overflows if this size (or "maxroom") is negative;
3/ integer overflows (for example, "*maxcells+1");
4/ always null-terminate the destination buffer (unless its size is 0).
---------------------------- adapted for newlib branch
. the escape.c now has just a single exported function
. thus SECURE_ESCAPE_ARGS() is needed in only 2 places
. unlike that original patch, macro is executed 1 time
( not like 'escape_command' calling 'escape_strlist' )
( which might then call 'escape_str' multiple times! )
Signed-off-by: Jim Warner <james.warner@comcast.net>
In the human_readable case; otherwise the strcat() that follows may
append bytes to the previous contents of buf.
Also, slightly enlarge buf, as it was a bit too tight.
Could also replace all sprintf()s with snprintf()s, but all the calls
here output a limited number of characters, so they should be safe.
---------------------------- adapted for newlib branch
. the source file is now proc/uptime.c
. function is now named 'procps_uptime_sprint()'
. new human readable function 'procps_uptime_sprint_short()'
. both were already initialized, so just raised size of 2 buffers
Signed-off-by: Jim Warner <james.warner@comcast.net>
In proc/slab.c, functions parse_slabinfo20() and parse_slabinfo11(),
sscanf() might overflow curr->name, because "String input conversions
store a terminating null byte ('\0') to mark the end of the input; the
maximum field width does not include this terminator."
Add one byte to name[] for this terminator.
---------------------------- adapted for newlib branch
. file is now proc/slabinfo.c (not .h)
. manifest constant renamed SLABINFO_NAME_LEN
. older parse_slabinfo11() function no longer present
Signed-off-by: Jim Warner <james.warner@comcast.net>
