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

[Shahab Vahedi <shahab.vahedi@gmail.com>]

On Mon, Jan 06, 2020 at 12:17:51PM +0000, Pedro Alves wrote:
> 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.

Indeed! Andrew mentioned the same to me. I will fix 'em.

>
> > 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.

I will submit the trailing spaces fix in a separate patch.

>
> > @@ -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.
>

How does this look?

   bool is_displayed = false;
   int threshold = SCROLL_THRESHOLD;

-  int i = 0;
-  while (i < content.size () - threshold && !is_displayed)
+  if (content.size () < threshold)
+    return is_displayed;
+
+  for (size_t i = 0; i < content.size () - threshold && !is_displayed; ++i)
     {
       is_displayed
        = (content[i].line_or_addr.loa == LOA_ADDRESS
           && content[i].line_or_addr.u.addr == addr);
-      i++;
     }

> Thanks,
> Pedro Alves
>
--
Shahab