diff mbox

[4/4] gdb: Avoid signed integer overflow when printing source lines

Message ID 20190109130326.GY3456@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Jan. 9, 2019, 1:03 p.m. UTC
* Tom Tromey <tom@tromey.com> [2019-01-08 18:08:27 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The solution in this patch is a new class source_lines_range that can
> Andrew> be constructed from a single line number and a direction (forward or
> Andrew> backward).  The range is then constructed from the line number and the
> Andrew> value of get_lines_to_list.
> 
> Sorry I didn't get to this the other night.
> 
> Andrew> +      source_lines_range range (sal_end.line + 1,
> Andrew> +				source_lines_range::direction::backward);
> 
> Normally I think gdb uses upper case for enumerators.
> 
> Andrew> +  /* When constructing the range from a single line number, does the line
> Andrew> +     range extend forward, or backward.  */
> Andrew> +  enum class direction
> Andrew> +  {
> Andrew> +   forward,
> Andrew> +   backward
> Andrew> +  };
> 
> For an enum that's already nested in a class, I would probably just use
> plain "enum" and not "enum class", since IMO the extra namespacing just
> results in noise -- "source_lines_range::backward" seems just as clear,
> but shorter, than "source_lines_range::direction::backward".
> 
> I don't insist on it though, I just thought I'd throw this out there to
> see what others think.  As far as I know we don't have a rule about
> this.
> 
> Andrew> +  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
> Andrew> +   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
> Andrew> +   direction is backward then the start is actually (STARTLINE -
> Andrew> +   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
> Andrew> +   is always 1 or more, and the end will be at most INT_MAX.  */
> Andrew> +  source_lines_range (int startline, direction dir = direction::forward);
> 
> I think this should be marked "explicit".  I guess it could go either
> way but seeing as the patch uses it in an explicit way, and since
> explicit is generally better...

Thanks for the review.

This iteration has:

  - Enum values with CAPITAL letters,
  - No longer using an enum class,
  - Added use of explicit on constructors.

Thanks,
Andrew

--

gdb: Avoid signed integer overflow when printing source lines

When printing source lines with calls to print_source_lines we need to
pass a start line number and an end line number.  The end line number
is calculated by calling get_lines_to_list and adding this value to
the start line number.  For example this code from list_command:

    print_source_lines (cursal.symtab, first,
                        first + get_lines_to_list (), 0);

The problem is that get_lines_to_list returns a value based on the
GDB setting `set listsize LISTSIZE`.  By default LISTSIZE is 10,
however, its also possible to set LISTSIZE to unlimited, in which
case get_lines_to_list will return INT_MAX.

As the parameter signature for print_source_lines is:

  void print_source_lines (struct symtab *, int, int,
                           print_source_lines_flags);

and `first` in the above code is an `int`, then when LISTSIZE is
`unlimited` the above code will result in signed integer overflow,
which is undefined.

The solution in this patch is a new class source_lines_range that can
be constructed from a single line number and a direction (forward or
backward).  The range is then constructed from the line number and the
value of get_lines_to_list.

gdb/ChangeLog:

	* cli/cli-cmds.c (list_command): Pass a source_lines_range to
	print_source_lines.
	* source.c (print_source_lines_base): Update line number check.
	(print_source_lines): New function.
	(source_lines_range::source_lines_range): New function.
	* source.h (class source_lines_range): New class.
	(print_source_lines): New declaration.
---
 gdb/ChangeLog      | 10 ++++++++++
 gdb/cli/cli-cmds.c | 35 ++++++++++++++++-------------------
 gdb/source.c       | 48 +++++++++++++++++++++++++++++++++++++++++-------
 gdb/source.h       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 26 deletions(-)

Comments

Tom Tromey Jan. 9, 2019, 1:40 p.m. UTC | #1
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This iteration has:
Andrew>   - Enum values with CAPITAL letters,
Andrew>   - No longer using an enum class,
Andrew>   - Added use of explicit on constructors.

This is ok.  Thanks for doing this.

Tom
diff mbox

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 9e09c05513c..57cfad441c9 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -895,14 +895,13 @@  list_command (const char *arg, int from_tty)
 	      && get_lines_to_list () == 1 && first > 1)
 	    first -= 1;
 
-	  print_source_lines (cursal.symtab, first,
-			      first + get_lines_to_list (), 0);
+	  print_source_lines (cursal.symtab, source_lines_range (first), 0);
 	}
 
       /* "l" or "l +" lists next ten lines.  */
       else if (arg == NULL || arg[0] == '+')
-	print_source_lines (cursal.symtab, cursal.line,
-			    cursal.line + get_lines_to_list (), 0);
+	print_source_lines (cursal.symtab,
+			    source_lines_range (cursal.line), 0);
 
       /* "l -" lists previous ten lines, the ones before the ten just
 	 listed.  */
@@ -911,10 +910,9 @@  list_command (const char *arg, int from_tty)
 	  if (get_first_line_listed () == 1)
 	    error (_("Already at the start of %s."),
 		   symtab_to_filename_for_display (cursal.symtab));
-	  print_source_lines (cursal.symtab,
-			      std::max (get_first_line_listed ()
-					- get_lines_to_list (), 1),
-			      get_first_line_listed (), 0);
+	  source_lines_range range (get_first_line_listed (),
+				    source_lines_range::BACKWARD);
+	  print_source_lines (cursal.symtab, range, 0);
 	}
 
       return;
@@ -1059,9 +1057,11 @@  list_command (const char *arg, int from_tty)
   if (dummy_beg && sal_end.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   if (dummy_beg)
-    print_source_lines (sal_end.symtab,
-			std::max (sal_end.line - (get_lines_to_list () - 1), 1),
-			sal_end.line + 1, 0);
+    {
+      source_lines_range range (sal_end.line + 1,
+				source_lines_range::BACKWARD);
+      print_source_lines (sal_end.symtab, range, 0);
+    }
   else if (sal.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   else if (no_end)
@@ -1074,17 +1074,14 @@  list_command (const char *arg, int from_tty)
 	    first_line = 1;
 	  if (sals.size () > 1)
 	    print_sal_location (sal);
-	  print_source_lines (sal.symtab,
-			      first_line,
-			      first_line + get_lines_to_list (),
-			      0);
+	  print_source_lines (sal.symtab, source_lines_range (first_line), 0);
 	}
     }
+  else if (dummy_end)
+    print_source_lines (sal.symtab, source_lines_range (sal.line), 0);
   else
-    print_source_lines (sal.symtab, sal.line,
-			(dummy_end
-			 ? sal.line + get_lines_to_list ()
-			 : sal_end.line + 1),
+    print_source_lines (sal.symtab,
+			source_lines_range (sal.line, (sal_end.line + 1)),
 			0);
 }
 
diff --git a/gdb/source.c b/gdb/source.c
index f865c8a9508..1f10379a071 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1331,13 +1331,8 @@  print_source_lines_base (struct symtab *s, int line, int stopline,
   last_source_error = 0;
 
   /* If the user requested a sequence of lines that seems to go backward
-     (from high to low line numbers) then we don't print anything.
-     The use of '- 1' here instead of '<=' is currently critical, we rely
-     on the undefined wrap around behaviour of 'int' for stopline.  When
-     the use has done: 'set listsize unlimited' then stopline can overflow
-     and appear as MIN_INT.  This is a long-standing bug that needs
-     fixing.  */
-  if (stopline - 1 < line)
+     (from high to low line numbers) then we don't print anything.  */
+  if (stopline <= line)
     return;
 
   std::string lines;
@@ -1399,6 +1394,18 @@  print_source_lines (struct symtab *s, int line, int stopline,
 {
   print_source_lines_base (s, line, stopline, flags);
 }
+
+/* See source.h.  */
+
+void
+print_source_lines (struct symtab *s, source_lines_range line_range,
+		    print_source_lines_flags flags)
+{
+  print_source_lines_base (s, line_range.startline (),
+			   line_range.stopline (), flags);
+}
+
+
 
 /* Print info on range of pc's in a specified line.  */
 
@@ -1822,6 +1829,33 @@  set_substitute_path_command (const char *args, int from_tty)
   forget_cached_source_info ();
 }
 
+/* See source.h.  */
+
+source_lines_range::source_lines_range (int startline,
+					source_lines_range::direction dir)
+{
+  if (dir == source_lines_range::FORWARD)
+    {
+      LONGEST end = static_cast <LONGEST> (startline) + get_lines_to_list ();
+
+      if (end > INT_MAX)
+	end = INT_MAX;
+
+      m_startline = startline;
+      m_stopline = static_cast <int> (end);
+    }
+  else
+    {
+      LONGEST start = static_cast <LONGEST> (startline) - get_lines_to_list ();
+
+      if (start < 1)
+	start = 1;
+
+      m_startline = static_cast <int> (start);
+      m_stopline = startline;
+    }
+}
+
 
 void
 _initialize_source (void)
diff --git a/gdb/source.h b/gdb/source.h
index fcd83daafcd..f1b5f6e8f7f 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -157,6 +157,54 @@  DEF_ENUM_FLAGS_TYPE (enum print_source_lines_flag, print_source_lines_flags);
 extern void print_source_lines (struct symtab *s, int line, int stopline,
 				print_source_lines_flags flags);
 
+/* Wrap up the logic to build a line number range for passing to
+   print_source_lines when using get_lines_to_list.  An instance of this
+   class can be built from a single line number and a direction (forward or
+   backward) the range is then computed using get_lines_to_list.  */
+class source_lines_range
+{
+public:
+  /* When constructing the range from a single line number, does the line
+     range extend forward, or backward.  */
+  enum direction
+  {
+   FORWARD,
+   BACKWARD
+  };
+
+  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
+   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
+   direction is backward then the start is actually (STARTLINE -
+   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
+   is always 1 or more, and the end will be at most INT_MAX.  */
+  explicit source_lines_range (int startline, direction dir = FORWARD);
+
+  /* Construct a SOURCE_LINES_RANGE from STARTLINE to STOPLINE.  */
+  explicit source_lines_range (int startline, int stopline)
+    : m_startline (startline),
+      m_stopline (stopline)
+  { /* Nothing.  */ }
+
+  /* Return the line to start listing from.  */
+  int startline () const
+  { return m_startline; }
+
+  /* Return the line after the last line that should be listed.  */
+  int stopline () const
+  { return m_stopline; }
+
+private:
+
+  /* The start and end of the range.  */
+  int m_startline;
+  int m_stopline;
+};
+
+/* Variation of previous print_source_lines that takes a range instead of a
+   start and end line number.  */
+extern void print_source_lines (struct symtab *s, source_lines_range r,
+				print_source_lines_flags flags);
+
 /* Forget line positions and file names for the symtabs in a
    particular objfile.  */
 extern void forget_cached_source_info_for_objfile (struct objfile *);