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

Shahab Vahedi
2020-01-06T12:43:59Z

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