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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Andrew Burgess <andrew.burgess@embecosm.com> [2016-01-20 16:22:57 +0100]:

> * 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)
>

I've had doc approval from Eli, and I believe I've addresses all of
the technical points raised by Pedro and Joel, I'm just hoping for
approval to merge this change.

Thanks,
Andrew




> 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