[v3] GDB: Fix the overflow in addr/line_is_displayed()

Message ID 20200106142732.32733-1-shahab.vahedi@gmail.com
State New
Headers show
Series
  • [v3] GDB: Fix the overflow in addr/line_is_displayed()
Related show

Commit Message

Shahab Vahedi Jan. 6, 2020, 2:27 p.m.
From: Shahab Vahedi <shahab@synopsys.com>


In tui_disasm_window::addr_is_displayed(), there can be situations
where "content" is empty. For instance, it can happen when the
"content" was not filled in tui_disasm_window::set_contents(),
because tui_disassemble() threw an exception. Usually this exception
is the result of fetching invalid PC addresses like the ones beyond
the end of the program.

Having "content.size ()" zero leads to an overflow in this condition
check inside tui_disasm_window::addr_is_displayed():

  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. Finally, "content[i]" access leads to
a segmentation fault.

Same problem applies to tui_source_window::line_is_displayed().

The issue has been discussed at length in bug 25345:
  https://sourceware.org/bugzilla/show_bug.cgi?id=25345

This commit avoids the segmentation faults with an early check:

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

Moreover, those functions have been overhauled to a leaner code.

gdb/ChangeLog:
2020-01-06  Shahab Vahedi  <shahab@synopsys.com>
        * tui/tui-disasm.c (tui_disasm_window::addr_is_displayed):
        Avoid overflow by an early check of content vs threshold.
        * tui/tui-source.c (tui_source_window::line_is_displayed):
        Likewise.
---
 gdb/tui/tui-disasm.c | 16 +++++++---------
 gdb/tui/tui-source.c | 17 ++++++++---------
 2 files changed, 15 insertions(+), 18 deletions(-)

-- 
2.24.1

Comments

Pedro Alves Jan. 6, 2020, 7:55 p.m. | #1
Hi,

I merged this patch without waiting for copyright clearing, since the
amount of new copyrightable code is a few lines only IMO (the threshold
check), and also the current form of the patch was largely suggested
by me in a previous email.

For further fixes, I think it's better to wait until copyright
issues are cleared though.

> This commit avoids the segmentation faults with an early check:

> 

>   if (contet.size () < SCROLL_THRESHOLD)

>     return false;


I fixed the "contet" typo before merging.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index ebd0ba317f5..98c691f3387 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -348,19 +348,17 @@  tui_disasm_window::location_matches_p (struct bp_location *loc, int line_no)
 bool
 tui_disasm_window::addr_is_displayed (CORE_ADDR addr) const
 {
-  bool is_displayed = false;
-  int threshold = SCROLL_THRESHOLD;
+  if (content.size () < SCROLL_THRESHOLD)
+    return false;
 
-  int i = 0;
-  while (i < content.size () - threshold && !is_displayed)
+  for (size_t i = 0; i < content.size () - SCROLL_THRESHOLD; ++i)
     {
-      is_displayed
-	= (content[i].line_or_addr.loa == LOA_ADDRESS
-	   && content[i].line_or_addr.u.addr == addr);
-      i++;
+      if (content[i].line_or_addr.loa == LOA_ADDRESS
+	  && content[i].line_or_addr.u.addr == addr)
+	return true;
     }
 
-  return is_displayed;
+  return false;
 }
 
 void
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index e028b724d23..1503cd4c636 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -174,18 +174,17 @@  tui_source_window::location_matches_p (struct bp_location *loc, int line_no)
 bool
 tui_source_window::line_is_displayed (int line) const
 {
-  bool is_displayed = false;
-  int threshold = SCROLL_THRESHOLD;
-  int i = 0;
-  while (i < content.size () - threshold && !is_displayed)
+  if (content.size () < SCROLL_THRESHOLD)
+    return false;
+
+  for (size_t i = 0; i < content.size () - SCROLL_THRESHOLD; ++i)
     {
-      is_displayed
-	= (content[i].line_or_addr.loa == LOA_LINE
-	   && content[i].line_or_addr.u.line_no == line);
-      i++;
+      if (content[i].line_or_addr.loa == LOA_LINE
+	  && content[i].line_or_addr.u.line_no == line)
+	return true;
     }
 
-  return is_displayed;
+  return false;
 }
 
 void