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

Pedro Alves
2020-01-06T12:18:06Z

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