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

* Shahab Vahedi <[email protected]> [2020-01-04 12:43:12 +0100]:

> From: Shahab Vahedi <[email protected]>
> 
> 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.
> This results into an overflow and the loop is entered whereas it
> should have been skipped.

I didn't quite understand the problem description.

I can see how 'content.size() - threshold' would overflow when the
size is 0 or 1, but I don't understand the part about the tui height
being bigger than the number of instructions.

I tried to reproduce the failure on native x86-64 but failed.  I can
get the initial stage reproduced, where the asm window is empty and I
see the error about "Cannot access memory at address ....", but then
when I attach to a remote the asm window fills in fine.  I guess this
is because on Linux the page the code is on is readable, and under
QEMU only the specific program instructions are readable maybe?

Anyway, the change looks reasonable, though I had one comment, inline
below...

> 
> This has been discussed at length in bug 25345:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=25345
> 
> gdb/ChangeLog:
> 2020-01-04  Shahab Vahedi  <[email protected]>
>         * tui/tui-disasm.c (tui_disasm_window::addr_is_displayed):
>         Treat "content.size()" as "int" to avoid overflow.
> ---
>  gdb/tui/tui-disasm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index c72b50730b0..a0921eb09d1 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -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()) - int(SCROLL_THRESHOLD);

I had a look through our code and I couldn't find any examples of us
using 'int (xxx)' syntax to cast to int, this is probably a result of
our C heritage, but we should probably try to be consistent I think.
I'd suggest maybe this become:

  int nr_of_lines = (int) content.size () - SCROLL_THRESHOLD;

Thanks,
Andrew

>  
>    int i = 0;
> -  while (i < content.size () - threshold && !is_displayed)
> +  while (i < nr_of_lines && !is_displayed)
>      {
>        is_displayed
>  	= (content[i].line_or_addr.loa == LOA_ADDRESS
> -- 
> 2.24.1
>