Date: Sun, 2 Jul 2023 11:44:10 -1000 From: Clifton Royston <cliftonr@volcano.org> To: freebsd-hackers@freebsd.org Subject: Re: top vs. array sorted_state's out of bounds indexing (and related issues) Message-ID: <f91ff360-5e91-e32a-6291-ef00d4123367@volcano.org> In-Reply-To: <2ad2965e-ccc4-a202-2a58-0b2970aad925@volcano.org> References: <DA51FDC7-EAF0-486E-9A85-22044D621E7D.ref@yahoo.com> <DA51FDC7-EAF0-486E-9A85-22044D621E7D@yahoo.com> <ZJB8t9TPLot6EkXU@kib.kiev.ua> <2ad2965e-ccc4-a202-2a58-0b2970aad925@volcano.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 7/2/2023 9:52 AM, Clifton Royston wrote:
> On 6/19/2023 6:05 AM, Konstantin Belousov wrote:
>> On Sun, Jun 18, 2023 at 09:23:06PM -0700, Mark Millard wrote:
>>> usr.bin/top/machine.c (from main) has:
[...]
>>>
>>> static int sorted_state[] = {
>>> 0, /* not used */
>>> 3, /* sleep */
>>> 1, /* ABANDONED (WAIT) */
>>> 6, /* run */
>>> 5, /* start */
>>> 2, /* zombie */
>>> 4 /* stop */
>>> };
>>>
>>> Note the elements are 0..6, so 7 elements.
>>>
>>> It is accessed via code like:
>>>
>>> sorted_state[(unsigned char)(b)->ki_stat]
> [...]
>>> /*
>>> * These were process status values (p_stat), now they are only
used in
>>> * legacy conversion code.
>>> */
>>> #define SIDL 1 /* Process being created by fork. */
>>> #define SRUN 2 /* Currently runnable. */
>>> #define SSLEEP 3 /* Sleeping on an address. */
>>> #define SSTOP 4 /* Process debugging or suspension. */
>>> #define SZOMB 5 /* Awaiting collection by parent. */
>>> #define SWAIT 6 /* Waiting for interrupt. */
>>> #define SLOCK 7 /* Blocked on a lock. */
>>>
> [...]
>>> } else if (TD_ON_LOCK(td)) {
>>> kp->ki_stat = SLOCK;
> [...]
>> https://reviews.freebsd.org/D40607
>
> I rarely comment here and hesitate to now, but out of curiosity I
> looked at the original report and at the chain of commits.
>
> It appears to me that in making the code more readable the final
> result may have accidentally inverted the sort order from the intended
> values (mostly.) A casual test wouldn't show this, as unless the sort
> order in top were changed, normally it would only come into play for
> two processes with equal CPU % and ticks which seems unlikely.
[...]
And, following up to myself, I now realized that both the committed
patch *and* my reply completely missed addressing the original issue
Mark reported:
No value for the index SLOCK is included in the array initializer, so
SLOCK is still not translated to a meaningful index, and AFAICT it is
not included in the dimensions! I expect it would still result in the
same out-of-bounds indexing problem Mark reported for that case.
A further corrected comment and array initializer:
/*
* proc_compare - comparison function for "qsort"
* Compares the resource consumption of two processes using five
* distinct keys. The keys (in descending order of importance) are:
* percent cpu, cpu ticks, state, resident set size, total virtual
* memory usage. The process states are ordered as follows (from most
* to least important): run, zombie, idle (being created), interrupt
* wait, blocked on lock, stop, sleep.
* The array declaration below maps a process state index into a
* number that reflects this ordering.
*/
static const int sorted_state[] = {
[SRUN] = 7, /* Currently runnable. */
[SZOMB] = 6, /* Awaiting collection by parent. */
[SIDL] = 5, /* Process being created by fork. */
[SWAIT] = 4, /* Waiting for interrupt. */
[SLOCK] = 3, /* Blocked on a lock. */
[SSTOP] = 2, /* Process debugging or suspension. */
[SSLEEP] = 1, /* Sleeping on an address. */
}
Best regards,
-- Clifton
--
Clifton Royston <cliftonr@volcano.org>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f91ff360-5e91-e32a-6291-ef00d4123367>