This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed()
- From: Shahab Vahedi <shahab dot vahedi at gmail dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Shahab Vahedi <shahab at synopsys dot com>, Claudiu Zissulescu <claziss at synopsys dot com>, Francois Bedard <fbedard at synopsys dot com>
- Date: Mon, 6 Jan 2020 13:43:53 +0100
- Subject: Re: [PATCH v2] GDB: Fix the overflow in addr_is_displayed()
- References: <20200106102649.15710-1-shahab.vahedi@gmail.com> <11035b53-bb43-740a-de38-6283062cdc6d@redhat.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