Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed()

[Pedro Alves <palves@redhat.com>]

On 1/6/20 10:26 AM, Shahab Vahedi wrote:
> From: Shahab Vahedi <shahab@synopsys.com>
> 
> In a corner case scenario, where the height of the assembly TUI is
> bigger than the number of instructions in the whole program, GDB
> dumps core. The problem roots in this condition check:
> 
>   int i = 0;
>   while (i < content. size() - threshold ...) {
>     ... content[i] ...
>   }
> 
> "threshold" is 2 and there are times that "content. size()" is 0.

Typo: spurious space in "content. size()", twice.  Should be "content.size ()"
instead.

> This results into an overflow and the loop is entered whereas it
> should have been skipped.
> 
> This has been discussed at length in bug 25345:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=25345
> 
> As a bonus, a few trailing spaces are also removed.

We try to avoid mixing unrelated formatting changes with
logical changes.  It would be better to push the whitespace
fixing as a separate patch.  Go ahead and merge that part
in as an obvious change.

> @@ -349,10 +349,10 @@ bool
>  tui_disasm_window::addr_is_displayed (CORE_ADDR addr) const
>  {
>    bool is_displayed = false;
> -  int threshold = SCROLL_THRESHOLD;
> +  int nr_of_lines = (int) content. size() - SCROLL_THRESHOLD;
>  

Someone reading this will have to think through the reason for
the int cast, since negative "number of lines" is a bit
nonsensical.  I suspect that instead doing an early return, like:

  if (content.size() < SCROLL_THRESHOLD)
    return false;

would end up being clearer?

That "while" loop could be a "for" too, no idea why it's not.

Thanks,
Pedro Alves