Re: top vs. array sorted_state's out of bounds indexing (and related issues)

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>