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

Andrew Burgess
2016-01-20T10:59:30Z

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

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2016-01-06 11:40:50 +0000]:

> * Eli Zaretskii <eliz@gnu.org> [2016-01-05 18:24:32 +0200]:
>
> > > Date: Tue, 5 Jan 2016 14:12:41 +0000
> > > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > > Cc: gdb-patches@sourceware.org
> > >
> > > 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.
> >
> > Thanks.
> >
> > > gdb/doc/ChangeLog:
> > >
> > > * gdb.texinfo (Value Sizes): New section, also add the new section
> > > to the menu.
> >
> > The addition to the menu is probably in a different node, so it needs
> > a separate entry in the ChangeLog.
> >
> > > +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.
> >
> > "Controls" and "Prevents". Also "maximum size of memory" (we allocate
> > memory, not size).
> >
> > > +@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
> >
> > Same here:
> >
> > Set the maximum size of memory, in bytes, that @value{GDBN} will ...
> >
> > Also, the "in bytes" part is redundant, since the parameter is called
> > "bytes", which is self-explanatory.
> >
> > > +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.
> >
> > I would remove the "can't be displayed by @value{GDBN}" part. It can
> > be interpreted to mean some limitation inherent in GDB, which is not
> > what you want to convey. The rest of the sentence says it all: 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 gdb, only future allocations.
> > ^^^
> > @value{GDBN}
> >
> > > +There's a minimum size that @code{max-value-size} can be set too in
> > ^^^
> > "to"
> >
> > OK with those fixed.
>
> New revision resolves all of Eli's comments, and also addresses
> Pedro's comment.
>
> OK to apply?
>
> 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/ChangeLog | 12 ++++
> gdb/NEWS | 6 ++
> gdb/doc/ChangeLog | 5 ++
> gdb/doc/gdb.texinfo | 42 +++++++++++++
> 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, 286 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..861b125 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
> + 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 ef3bdaa..97a1b25 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.
> + (Data): Add the 'Value Sizes' node 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..9dea873 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,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 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..41f44c8 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 %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
>