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

Andrew Burgess
2016-01-05T14:12:51Z

Eli included for docs review (with thanks).

* Joel Brobecker <brobecker@adacore.com> [2016-01-01 13:43:09 +0400]:

> On Fri, Dec 11, 2015 at 09:38:35PM +0000, Andrew Burgess wrote:
> > For languages with dynamic types, an incorrect 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
> > attempting to allocate an overly large buffer.
> >
> > This could result in the allocation failing, and gdb terminating, or the
> > buffer might be allocated, but the machine on which gdb is being run
> > might struggle due to the high memory requirement of gdb.
> >
> > 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 in bytes that the contents of a
> > value may allocate. Any attempt to allocate a value with size greater
> > than this will result in an error. The default for this variable is
> > currently unlimited.
> >
> > Setting the default to unlimited reduces the use of this variable a
> > little bit, if a user hits one of these rogue types without realising
> > it, then gdb will still allocate the large buffer, with all the problems
> > that this entails. The user will then be forced to probably restart
> > their gdb session, and then set the 'max-value-size' variable to guard
> > against future crashes.
> >
> > However, it's not clear what a good default would actually be, given the
> > huge range of resources that are available on different machines. One
> > solution might be to introduce platform specific code that attempts to
> > figure out what memory resources are available on the machine running
> > gdb, we could then set the max-value-size to some percentage of the
> > available memory, while still defaulting to unlimited for those machines
> > where we can't figure out a good alternative. Such a feature is not
> > implemented in this commit, but could be added later.
>
> I think it is important to activate this feature by default, precisely
> for the reasons you mention. It sounds like one of the reasons why
> you elected not to provide a default is that you weren't sure about
> what value to use as the default. But, luckily, AdaCore has been using
> 65536 for as long as I remember as its default varsize_limit. We have
> customers of all kinds, and I don't remember anyone telling us that
> the default limit is too small.
>
> I think a default of 65536 is a good start. It's large enough that
> I can't see any user wanting to display an object that large.
> But it's small enough that it'll prevent GDB from unexpectedly
> allocate such a chunk of memory that it'll take the user's GDB
> session down.

Thanks for the guidance, I've adopted 64k as the default as you suggest.

>
> By the way, here is the documentation we have in our manual regarding
> this setting. Part of it was prompted by customer feedback. In
> particular, it touches on something that made me think when
> looking at this new setting: how this limit is applied can be
> fairly obscure, when you think about it from the user's perspective.
> For instance, when you consider the following expression...
>
> (gdb) print huge_array[2]
>
> ... it is likely that the expression evaluation will first create
> a struct value for huge_array. Fortunately, the evalutor is smart
> and creates a lazy value, meaning that the limit does not kick in.
> Therefore, the limit would only apply to the element of that huge_array,
> which hopefully for the user is small enough to pass that check.
>
> | @table @code
> | @kindex set varsize-limit
> | @item set varsize-limit @var{size}
> | Limit the size of the types of objects to @var{size} bytes
> | when those sizes are computed from run-time quantities.
> | When this limit is set to 0, there is no
> | limit. By default, it is about 65K. The purpose of having such a limit is
> | to prevent @value{GDBN} from trying to grab enormous chunks of virtual
> | memory when asked to evaluate a quantity whose bounds have been corrupted
> | or have not yet been fully initialized.
> | 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{size}.
> | 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{size}.

Thanks. I've stolen some of these words almost verbatim into my
patch, I assume that's not a problem.

>
> > gdb/ChangeLog:
> >
> > * value.c (max_value_size): New variable.
> > (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.
> >
> > gdb/testsuite/ChangeLog:
> >
> > * gdb.base/max-value-size.c: New file.
> > * gdb.base/max-value-size.exp: New file.
>
> You probably already know this, but the NEWS and doco parts are
> reviewed by Eli (FAOD).
>
> > +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.
>
> (not just an invalid program; also uninitialized variables);
> you can use our documentation as inspiration, if you'd like.

I've updated the documentation to better explain this (I hope).

All the remaining issues were minor style issues which I believe are
all now addresses in the revised patch below.

Thanks, for the review.

---

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 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, also add the new section
to the menu.

gdb/testsuite/ChangeLog:

* gdb.base/max-value-size.c: New file.
* gdb.base/max-value-size.exp: New file.
---
gdb/ChangeLog | 12 ++++
gdb/NEWS | 6 ++
gdb/doc/ChangeLog | 5 ++
gdb/doc/gdb.texinfo | 43 ++++++++++++++
gdb/testsuite/ChangeLog | 5 ++
gdb/testsuite/gdb.base/max-value-size.c | 26 +++++++++
gdb/testsuite/gdb.base/max-value-size.exp | 97 +++++++++++++++++++++++++++++++
gdb/value.c | 97 +++++++++++++++++++++++++++++--
8 files changed, 287 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 3d8923b..42633d2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2016-01-05 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-04 Markus Metzger <markus.t.metzger@intel.com>

* btrace.c (btrace_pt_readmem_callback): Do not return in TRY/CATCH.
diff --git a/gdb/NEWS b/gdb/NEWS
index 484d98d..332808e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -60,6 +60,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
+ Control the maximum size, in bytes, that GDB will allocate for value
+ contents. Prevent incorrect programs from causing GDB to allocate
+ overly large buffers. Default is unlimited.
+
* 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 ef3bdaa..1465f6d 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2016-01-05 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ * gdb.texinfo (Value Sizes): New section, also add the new section
+ to the menu.
+
2015-12-11 Don Breazeal <donb@codesourcery.com>

* gdb.texinfo (Forks): Correct Linux kernel version where
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0778383..ae4b4ed 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8531,6 +8531,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
@@ -11596,6 +11597,48 @@ $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, in bytes, that @value{GDBN} will allocate for
+the contents of a value to @var{bytes}. Any value whose contents
+require more than this number of bytes can't be displayed by
+@value{GDBN}, and trying to display the value will result in an error.
+
+Setting this variable does not effect values that have already been
+allocated within gdb, only future allocations.
+
+There's a minimum size that @code{max-value-size} can be set too 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, 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 45a6c6a..b10da8c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2016-01-05 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ * gdb.base/max-value-size.c: New file.
+ * gdb.base/max-value-size.exp: New file.
+
2016-01-04 Markus Metzger <markus.t.metzger@intel.com>

* gdb.btrace/dlopen.exp: New.
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..41a5513 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 always greater than 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
+
+/* 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);
+ gdb_assert (sizeof (LONGEST) < MIN_VALUE_FOR_MAX_VALUE_SIZE);
+
+ 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 %d bytes, which is more "
+ "than max-value-size"), TYPE_NAME (type), length);
+ else
+ error (_("value requires %d 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