Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stesser
Copy link
Contributor

@stesser stesser commented Nov 13, 2021

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.

Fix program crash if the "m" command is used and "current" is higher than supported by the current stats array length.
Reset stats_history[] when switching the transform function - AFAICT the contents of the stats_history array depends on the transformation performed on the gathered statistics data.
Improve response time of "m" command to switch between device centric and measurement centric views
@chadmiller
Copy link
Owner

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?

@stesser
Copy link
Contributor Author

stesser commented Nov 14, 2021

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.

@chadmiller
Copy link
Owner

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

@@ -225,14 +225,19 @@ def render_stats(window, transform, should_show_differential, pool, filename=Non
elif in_key == ord('d'):
should_show_differential = not should_show_differential
elif in_key == ord('m'):
stats = None
stats_history = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of discarding recent history, I'd prefer it if stats and stats_history were unaffected by views and transforming functions were only used to interpret the raw data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not want to clear the stats_history array, I'd assume that you need two arrays, one for the device centric view, the other for the measurements centric view.
Always applying both transforms and just selecting one of 2 stats and stats_history arrays for display would be a simple change, but my main intend was to provide a fix for the issue that resulted from the addition of the "m" command.
If you provide 2 sets of arrays and put the restriction of "current" to the allowed index range at the start of the loop, then resetting of current to 0 could also be dropped.
I'll update the FreeBSD port to whatever you decide to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants