diff mbox

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

Message ID 20200106102649.15710-1-shahab.vahedi@gmail.com
State New
Headers show

Commit Message

Shahab Vahedi Jan. 6, 2020, 10:26 a.m. UTC
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.
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.

gdb/ChangeLog:
2020-01-04  Shahab Vahedi  <shahab@synopsys.com>
        * tui/tui-disasm.c (tui_disasm_window::addr_is_displayed):
        Treat "content.size ()" as "int" to avoid overflow.
        * tui/tui-disasm.c: Remove trailing spaces.
---
 gdb/tui/tui-disasm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Pedro Alves Jan. 6, 2020, 12:17 p.m. UTC | #1
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
Shahab Vahedi Jan. 6, 2020, 12:43 p.m. UTC | #2
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
Pedro Alves Jan. 6, 2020, 1:03 p.m. UTC | #3
On 1/6/20 12:43 PM, Shahab Vahedi wrote:

> 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;
> +

I'd write "false" instead of "is_displayed", to remove the
indirection.  Actually, do we really need the "threshold"
variable, btw?   Or even, "is_displayed"?  Isn't the
following equivalent?

  if (content.size () < SCROLL_THRESHOLD)
    return false;

  for (size_t i = 0; 
       i < content.size () - SCROLL_THRESHOLD; 
       i++)
    {
      if (content[i].line_or_addr.loa == LOA_ADDRESS
	  && content[i].line_or_addr.u.addr == addr)
        return true;
    }

  return false;

Anyway, what you have is fine too.

More importantly, doesn't tui_source_window::line_is_displayed
have the exact same issue?

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index c72b50730b0..68744cc61e3 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -43,7 +43,7 @@ 
 
 #include "gdb_curses.h"
 
-struct tui_asm_line 
+struct tui_asm_line
 {
   CORE_ADDR addr;
   std::string addr_string;
@@ -150,7 +150,7 @@  tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
       CORE_ADDR last_addr;
       int pos;
       struct bound_minimal_symbol msymbol;
-              
+
       /* Find backward an address which is a symbol and for which
          disassembling from that address will fill completely the
          window.  */
@@ -176,7 +176,7 @@  tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
         do
           {
             CORE_ADDR next_addr;
-                 
+
             pos++;
             if (pos >= max_lines)
               pos = 0;
@@ -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;
 
   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