Re: PING: Re: [PATCH 1/3] gdb: New set/show max-value-size command.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Andrew Burgess <andrew.burgess@embecosm.com> [2016-01-20 11:59:21 +0100]:

> Ping!  I believe I've addressed all review comments.

Following up on this again, I have another version of this patch below
which has two changes:

  - Switch from using '%d' to '%u' to display an unsigned int in the
    error message..

  - Set max-value-size to 'unlimited' for the test gdb.base/huge.exp.

I've tested this patch, and patch 3/3 [1] against the gdb testsuite on
x86-64 linux, on a machine that has gfortran 5.3.1 installed.  I do
see the max-value-size exceeded error message crop up in 6 fortran
tests, though I think that these are all legitimate errors,
representing undefined, potentially unsafe behaviour being prevented.

[1]  https://sourceware.org/ml/gdb-patches/2015-12/msg00247.html)

Thanks,
Andrew

---

For languages with dynamic types, an incorrect program, or uninitialised
variables within a program, could result in an incorrect, overly large
type being associated with a value.  Currently, attempting to print such
a variable will result in gdb trying to allocate an overly large buffer.

If this large memory allocation fails then the result can be gdb either
terminating, or (due to memory contention) becoming unresponsive for the
user.

A new user visible variable in gdb helps guard against such problems,
two new commands are available:

   set max-value-size
   show max-value-size

The 'max-value-size' is the maximum size of memory in bytes that gdb
will allocate for the contents of a value.  Any attempt to allocate a
value with a size greater than this will result in an error.  The
initial default for this limit is set at 64k, this is based on a similar
limit that exists within the ada specific code.

It is possible for the user to set max-value-size to unlimited, in which
case the old behaviour is restored.

gdb/ChangeLog:

	* value.c (max_value_size): New variable.
	(MIN_VALUE_FOR_MAX_VALUE_SIZE): New define.
	(show_max_value_size): New function.
	(check_type_length_before_alloc): New function.
	(allocate_value_contents): Call check_type_length_before_alloc.
	(set_value_enclosing_type): Likewise.
	(_initialize_values): Add set/show handler for max-value-size.
	* NEWS: Mention new set/show command.

gdb/doc/ChangeLog:

	* gdb.texinfo (Value Sizes): New section.
	(Data): Add the 'Value Sizes' node to the menu.

gdb/testsuite/ChangeLog:

	* gdb.base/max-value-size.c: New file.
	* gdb.base/max-value-size.exp: New file.
	* gdb.base/huge.exp: Disable max-value-size for this test.
---
 gdb/ChangeLog                             | 12 ++++
 gdb/NEWS                                  |  6 ++
 gdb/doc/ChangeLog                         |  5 ++
 gdb/doc/gdb.texinfo                       | 42 +++++++++++++
 gdb/testsuite/ChangeLog                   |  6 ++
 gdb/testsuite/gdb.base/huge.exp           |  2 +
 gdb/testsuite/gdb.base/max-value-size.c   | 26 +++++++++
 gdb/testsuite/gdb.base/max-value-size.exp | 97 +++++++++++++++++++++++++++++++
 gdb/value.c                               | 97 +++++++++++++++++++++++++++++--
 9 files changed, 289 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/max-value-size.c
 create mode 100644 gdb/testsuite/gdb.base/max-value-size.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d1a6069..5cb98fb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2016-01-20  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* value.c (max_value_size): New variable.
+	(MIN_VALUE_FOR_MAX_VALUE_SIZE): New define.
+	(set_max_value_size): New function.
+	(show_max_value_size): New function.
+	(check_type_length_before_alloc): New function.
+	(allocate_value_contents): Call check_type_length_before_alloc.
+	(set_value_enclosing_type): Likewise.
+	(_initialize_values): Add set/show handler for max-value-size.
+	* NEWS: Mention new set/show command.
+
 2016-01-20  Joel Brobecker  <brobecker@adacore.com>
 
 	* printcmd.c (print_scalar_formatted): move binary operator from
diff --git a/gdb/NEWS b/gdb/NEWS
index 4312117..962eae4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -119,6 +119,12 @@ show ada print-signatures"
   Control whether parameter types and return types are displayed in overloads
   selection menus.  It is activaled (@code{on}) by default.
 
+set max-value-size
+show max-value-size
+  Controls the maximum size of memory, in bytes, that GDB will
+  allocate for value contents.  Prevents incorrect programs from
+  causing GDB to allocate overly large buffers.  Default is 64k.
+
 * The "disassemble" command accepts a new modifier: /s.
   It prints mixed source+disassembly like /m with two differences:
   - disassembled instructions are now printed in program order, and
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 990f2ec..83710c0 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2016-01-20  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.texinfo (Value Sizes): New section.
+	(Data): Add the 'Value Sizes' node to the menu.
+
 2016-01-19  John Baldwin  <jhb@FreeBSD.org>
 
 	* gdb.texinfo (Debugging Output): Document "set/show debug fbsd-lwp".
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index cd311db..27db877 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8636,6 +8636,7 @@ being passed the type of @var{arg} as the argument.
                                 character set than GDB does
 * Caching Target Data::         Data caching for targets
 * Searching Memory::            Searching memory for a sequence of bytes
+* Value Sizes::                 Managing memory allocated for values
 @end menu
 
 @node Expressions
@@ -11711,6 +11712,47 @@ $1 = 1
 $2 = (void *) 0x8049560
 @end smallexample
 
+@node Value Sizes
+@section Value Sizes
+
+Whenever @value{GDBN} prints a value memory will be allocated within
+@value{GDBN} to hold the contents of the value.  It is possible in
+some languages with dynamic typing systems, that an invalid program
+may indicate a value that is incorrectly large, this in turn may cause
+@value{GDBN} to try and allocate an overly large ammount of memory.
+
+@table @code
+@kindex set max-value-size
+@itemx set max-value-size @var{bytes}
+@itemx set max-value-size unlimited
+Set the maximum size of memory that @value{GDBN} will allocate for the
+contents of a value to @var{bytes}, trying to display a value that
+requires more memory than that will result in an error.
+
+Setting this variable does not effect values that have already been
+allocated within @value{GDBN}, only future allocations.
+
+There's a minimum size that @code{max-value-size} can be set to in
+order that @value{GDBN} can still operate correctly, this minimum is
+currently 16 bytes.
+
+The limit applies to the results of some subexpressions as well as to
+complete expressions.  For example, an expression denoting a simple
+integer component, such as @code{x.y.z}, may fail if the size of
+@var{x.y} is dynamic and exceeds @var{bytes}.  On the other hand,
+@value{GDBN} is sometimes clever; the expression @code{A[i]}, where
+@var{A} is an array variable with non-constant size, will generally
+succeed regardless of the bounds on @var{A}, as long as the component
+size is less than @var{bytes}.
+
+The default value of @code{max-value-size} is currently 64k.
+
+@kindex show max-value-size
+@item show max-value-size
+Show the maximum size of memory, in bytes, that @value{GDBN} will
+allocate for the contents of a value.
+@end table
+
 @node Optimized Code
 @chapter Debugging Optimized Code
 @cindex optimized code, debugging
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b96a4ed..f440ef4 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2016-01-20  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.base/max-value-size.c: New file.
+	* gdb.base/max-value-size.exp: New file.
+	* gdb.base/huge.exp: Disable max-value-size for this test.
+
 2016-01-19  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* Makefile.in (DO_RUNTEST): Add --status and update usages.
diff --git a/gdb/testsuite/gdb.base/huge.exp b/gdb/testsuite/gdb.base/huge.exp
index 018f305..4dca89b 100644
--- a/gdb/testsuite/gdb.base/huge.exp
+++ b/gdb/testsuite/gdb.base/huge.exp
@@ -47,6 +47,8 @@ if { ! [ runto_main ] } then {
     return -1
 }
 
+gdb_test_no_output "set max-value-size unlimited"
+
 gdb_test "print a" ".1 = .0 .repeats \[0123456789\]+ times.." "print a very large data object"
 
 set timeout $prev_timeout
diff --git a/gdb/testsuite/gdb.base/max-value-size.c b/gdb/testsuite/gdb.base/max-value-size.c
new file mode 100644
index 0000000..b6a6fe5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/max-value-size.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+char one;
+char ten [10];
+char one_hundred [100];
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/max-value-size.exp b/gdb/testsuite/gdb.base/max-value-size.exp
new file mode 100644
index 0000000..63a97a0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/max-value-size.exp
@@ -0,0 +1,97 @@
+# Copyright (C) 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+# Run "show max-value-size" and return the interesting bit of the
+# result.  This is either the maximum size in bytes, or the string
+# "unlimited".
+proc get_max_value_size {} {
+    global gdb_prompt
+    global decimal
+
+    gdb_test_multiple "show max-value-size" "" {
+	-re "Maximum value size is ($decimal) bytes.*$gdb_prompt $" {
+	    return $expect_out(1,string)
+	}
+	-re "Maximum value size is unlimited.*$gdb_prompt $" {
+	    return "unlimited"
+	}
+    }
+}
+
+# Assuming that MAX_VALUE_SIZE is the current value for
+# max-value-size, print the test values.  Use TEST_PREFIX to make the
+# test names unique.
+proc do_value_printing { max_value_size test_prefix } {
+    with_test_prefix ${test_prefix} {
+	gdb_test "p/d one" " = 0"
+	if { $max_value_size != "unlimited" && $max_value_size < 100 } then {
+	    gdb_test "p/d one_hundred" \
+		"value requires 100 bytes, which is more than max-value-size"
+	} else {
+	    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
+	}
+	gdb_test "p/d one_hundred \[99\]" " = 0"
+    }
+}
+
+# Install SET_VALUE as the value for max-value-size, then print the
+# test values.
+proc set_and_check_max_value_size { set_value } {
+    if { $set_value == "unlimited" } then {
+	set check_pattern "unlimited"
+    } else {
+	set check_pattern "${set_value} bytes"
+    }
+
+    gdb_test_no_output "set max-value-size ${set_value}"
+    gdb_test "show max-value-size" \
+	"Maximum value size is ${check_pattern}." \
+	"check that the value shows as ${check_pattern}"
+
+    do_value_printing ${set_value} "max-value-size is '${set_value}'"
+}
+
+# Check the default value is sufficient.
+do_value_printing [get_max_value_size] "using initial max-value-size"
+
+# Check some values for max-value-size that should prevent some
+# allocations.
+set_and_check_max_value_size 25
+set_and_check_max_value_size 99
+
+# Check values for max-value-size that should allow all allocations.
+set_and_check_max_value_size 100
+set_and_check_max_value_size 200
+set_and_check_max_value_size "unlimited"
+
+# Check that we can't set the maximum size stupidly low.
+gdb_test "set max-value-size 1" \
+    "max-value-size set too low, increasing to \[0-9\]+ bytes"
+gdb_test "set max-value-size 0" \
+    "max-value-size set too low, increasing to \[0-9\]+ bytes"
+gdb_test "set max-value-size -5" \
+    "only -1 is allowed to set as unlimited"
diff --git a/gdb/value.c b/gdb/value.c
index eeb2b7d..aa551ae 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -955,13 +955,86 @@ allocate_value_lazy (struct type *type)
   return val;
 }
 
+/* The maximum size, in bytes, that GDB will try to allocate for a value.
+   The initial value of 64k was not selected for any specific reason, it is
+   just a reasonable starting point.  */
+
+static int max_value_size = 65536; /* 64k bytes */
+
+/* It is critical that the MAX_VALUE_SIZE is at least as bit as the size of
+   LONGEST, otherwise GDB will not be able to parse integer values from the
+   CLI; for example if the MAX_VALUE_SIZE could be set to 1 then GDB would
+   be unable to parse "set max-value-size 2".
+
+   As we want a consistent GDB experience across hosts with different sizes
+   of LONGEST, this arbitrary minimum value was selected, so long as this
+   is bigger than LONGEST of all GDB supported hosts we're fine.  */
+
+#define MIN_VALUE_FOR_MAX_VALUE_SIZE 16
+gdb_static_assert (sizeof (LONGEST) <= MIN_VALUE_FOR_MAX_VALUE_SIZE);
+
+/* Implement the "set max-value-size" command.  */
+
+static void
+set_max_value_size (char *args, int from_tty,
+		    struct cmd_list_element *c)
+{
+  gdb_assert (max_value_size == -1 || max_value_size >= 0);
+
+  if (max_value_size > -1 && max_value_size < MIN_VALUE_FOR_MAX_VALUE_SIZE)
+    {
+      max_value_size = MIN_VALUE_FOR_MAX_VALUE_SIZE;
+      error (_("max-value-size set too low, increasing to %d bytes"),
+	     max_value_size);
+    }
+}
+
+/* Implement the "show max-value-size" command.  */
+
+static void
+show_max_value_size (struct ui_file *file, int from_tty,
+		     struct cmd_list_element *c, const char *value)
+{
+  if (max_value_size == -1)
+    fprintf_filtered (file, _("Maximum value size is unlimited.\n"));
+  else
+    fprintf_filtered (file, _("Maximum value size is %d bytes.\n"),
+		      max_value_size);
+}
+
+/* Called before we attempt to allocate or reallocate a buffer for the
+   contents of a value.  TYPE is the type of the value for which we are
+   allocating the buffer.  If the buffer is too large (based on the user
+   controllable setting) then throw an error.  If this function returns
+   then we should attempt to allocate the buffer.  */
+
+static void
+check_type_length_before_alloc (const struct type *type)
+{
+  unsigned int length = TYPE_LENGTH (type);
+
+  if (max_value_size > -1 && length > max_value_size)
+    {
+      if (TYPE_NAME (type) != NULL)
+	error (_("value of type `%s' requires %u bytes, which is more "
+		 "than max-value-size"), TYPE_NAME (type), length);
+      else
+	error (_("value requires %u bytes, which is more than "
+		 "max-value-size"), length);
+    }
+}
+
 /* Allocate the contents of VAL if it has not been allocated yet.  */
 
 static void
 allocate_value_contents (struct value *val)
 {
   if (!val->contents)
-    val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+    {
+      check_type_length_before_alloc (val->enclosing_type);
+      val->contents
+	= (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+    }
 }
 
 /* Allocate a  value  and its contents for type TYPE.  */
@@ -2986,9 +3059,12 @@ value_static_field (struct type *type, int fieldno)
 void
 set_value_enclosing_type (struct value *val, struct type *new_encl_type)
 {
-  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val))) 
-    val->contents =
-      (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+  if (TYPE_LENGTH (new_encl_type) > TYPE_LENGTH (value_enclosing_type (val)))
+    {
+      check_type_length_before_alloc (new_encl_type);
+      val->contents
+	= (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+    }
 
   val->enclosing_type = new_encl_type;
 }
@@ -4013,4 +4089,17 @@ Check whether an expression is void.\n\
 Usage: $_isvoid (expression)\n\
 Return 1 if the expression is void, zero otherwise."),
 			 isvoid_internal_fn, NULL);
+
+  add_setshow_zuinteger_unlimited_cmd ("max-value-size",
+				       class_support, &max_value_size, _("\
+Set maximum sized value gdb will load from the inferior."), _("\
+Show maximum sized value gdb will load from the inferior."), _("\
+Use this to control the maximum size, in bytes, of a value that gdb\n\
+will load from the inferior.  Setting this value to 'unlimited'\n\
+disables checking.\n\
+Setting this does not invalidate already allocated values, it only\n\
+prevents future values, larger than this size, from being allocated."),
+			    set_max_value_size,
+			    show_max_value_size,
+			    &setlist, &showlist);
 }
-- 
2.5.1