zpool-iostat-viz icon indicating copy to clipboard operation
zpool-iostat-viz copied to clipboard

Fix program crash on "m" with large value of "current"

Open stesser opened this issue 3 years ago • 3 comments

Fix program crash if the "m" command is used and "current" is higher than supported by the current stats array length.

The "m" command has been part of a previous pull request that has been merged. I forgot that the stats array length can change and that the stats_history contents depends on the transform function selected.

This patch fixes this issue. The value of "current" is reset to one that is valid for all possible stats array lengths, and stats_history is reset to an empty array to prevent incompatible data to be used in a difference calculation.

stesser avatar Nov 13 '21 22:11 stesser

I don't have a bug report so I don't know what it's fixing yet. The stats array can change? As in, the device inventory can change, or the dimensions of measurement can change?

chadmiller avatar Nov 14 '21 02:11 chadmiller

The value of "current" is limited to (0 .. length(stats)-1) at the end of the loop. But when "m" is used to switch the mode, "current" may be too large for the array. To reproduce: the value of "current" can be 0 to 9 for the measurement centric view (10 measurements). If you have less than 10 device screens, then selecting measurement 10 and pressing "m" leads to a program abort due to an out-of-bounds access. OTOH, if there are more than 10 device screens, selecting the last one and pressing "m" will try to display a measurement screen > 10 which does not exist. Simple test case: start with or without "-d", press "left" to go the highest numbered screen, press "m". If the target screen exists press the "right" key to select a screen one higher than before and press "m" again. Only if there are exactly 10 device "pages" (i.e. "current" goes from 0 to 9 for devices, too), the program will not crash.

The pull request consists of 3 parts:

  1. reset current to 0 when switching display modes (save default screen number known to exist)
  2. reset stats_history array to empty (since it contains mode dependent information, AFAICT, which does not give useful deltas after switching the display mode)
  3. Force immediate refresh of the displayed values (else "m" causes a jump to screen 1 at first, then a switch to the new display mode after a delay)

I had missed the fact that current must be within bounds of an array that changes size depending on the device vs. measurements display mode when submitting my previous pull request.

These changes make the use of "m" safe and immediately resulting in the selected display mode.

stesser avatar Nov 14 '21 08:11 stesser

Ah.

  File "bin/zpool-iostat-viz", line 307, in <module>
    raise exc
  File "bin/zpool-iostat-viz", line 296, in <module>
    curses.wrapper(lambda window: main(window, parsed_args["diff"], parsed_args["parts"], parsed_args["file"], parsed_args["by"] or "m"))
  File "/usr/lib/python3.8/curses/__init__.py", line 105, in wrapper
    return func(stdscr, *args, **kwds)
  File "bin/zpool-iostat-viz", line 296, in <lambda>
    curses.wrapper(lambda window: main(window, parsed_args["diff"], parsed_args["parts"], parsed_args["file"], parsed_args["by"] or "m"))
  File "bin/zpool-iostat-viz", line 251, in main
    render_stats(window, transformation_function, should_show_differential, pool, filename)
  File "bin/zpool-iostat-viz", line 141, in render_stats
    name, rows  = stats[current]
IndexError: list index out of range

chadmiller avatar Nov 14 '21 17:11 chadmiller