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

[Joel Brobecker <brobecker at adacore dot com>]

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.

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}.

> 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.

> 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..4d47280
> --- /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 2015 Free Software Foundation, Inc.

You'll need to change the year to be 2015-2016 for all the files
your patch adds :-).  Also, while at it, I think the FSF prefers
the (C) to be there. Thus:

   Copyright (C) 2015-2016 Free Software Foundation, Inc.

> +char one;
> +char ten [10];
> +char one_hundred [100];
> +
> +int
> +main ()

[we decided that, as much as possible, example programs in the testsuite
would also follow the GDB Coding Style]

Can you change "main ()" to "main (void)"?

> +with_test_prefix "max-value-size is 'unlimited'" {
> +    gdb_test "p/d one" " = 0"
> +    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
> +}
> +
> +# Check that setting it low does prevent values being allocated.
> +gdb_test_no_output "set max-value-size 25"
> +with_test_prefix "max-value-size is '25'" {
> +    gdb_test "p/d one" " = 0"
> +    gdb_test "p/d one_hundred" "value contents too large \\(100 bytes\\)"

> +# Check we can set it to something "large", and then view our values.
> +gdb_test_no_output "set max-value-size 200"
> +gdb_test "show max-value-size" \
> +    "Maximum value size is 200 bytes." \
> +    "check that the value shows as 200 bytes"
> +with_test_prefix "max-value-size is '200'" {
> +    gdb_test "p/d one" " = 0"
> +    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
> +}
> +
> +# Check we can restore it too unlimited.
> +gdb_test_no_output "set max-value-size unlimited"
> +gdb_test "show max-value-size" \
> +    "Maximum value size is unlimited." \
> +    "check that the unlimited maximum restored correctly"
> +with_test_prefix "max-value-size is 'unlimited' again" {
> +    gdb_test "p/d one" " = 0"
> +    gdb_test "p/d one_hundred" " = \\{0 <repeats 100 times>\\}"
> +}
In all "with_test_prefix" cases, I would add a test to print
one element of one_hundred. This is particularly interesting
in the second situation, where although we can't print one_hundred
in its entirety, we can still print one element. This will help
catch situations where an array value is unexpectedly fetched.

> diff --git a/gdb/value.c b/gdb/value.c
> index 91bf49e..9554333 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -955,13 +955,60 @@ allocate_value_lazy (struct type *type)
>    return val;
>  }
>  
> +/* The maximum size, in bytes, that the contents of a value might have.
> +   Setting this to -1 indicates unlimited.  Adjust this variable does not
> +   invalidate already allocated values, only prevents future large values
> +   being allocated.  */
> +
> +static int max_value_size = -1;

What I would do, to document this static global, is just reference
the user-visible setting.  That way, you do not have to repeat
the user documentation.

Please add an empty line after this variable, to separate it from
the rest of the code.

> +static void
> +set_max_value_size (char *args, int from_tty,
> +		    struct cmd_list_element *c)

Every new function should be documented individually. In this case,
it's implementing the "set max-value-size" command, so it's sufficient
to just say that in the function's intro comment. Eg:

/* Implement the "set max-value-size" command.  */

> +{
> +  if (max_value_size > -1 && max_value_size < sizeof (LONGEST))
> +    {
> +      max_value_size = sizeof (LONGEST);
> +      error (_("max-value-size set too low, increasing to %u bytes"),
> +	     ((unsigned int) max_value_size));
> +    }

sizeof (LONGEST) depends on the host, so it opens the door for
GDB behaving differently depending on the host it runs on, which
can be surprising.

I suggest either: remove the minimum limit entirely, or just choose
an arbitrary one (Eg: 8).

Also, why are you printing max_value_size as an unsigned int, rather
than an int in the error message? This forces us to add a cast, and
its purpose is unclear to me.

> +static void
> +show_max_value_size (struct ui_file *file, int from_tty,
> +		     struct cmd_list_element *c, const char *value)

Same as above; a trivial function intro is suffficient.

> +/* 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)
> +{
> +  int length = TYPE_LENGTH (type);
> +  if (max_value_size > -1 && (length < 0 || length > max_value_size))
> +    error (_("value contents too large (%u bytes)"),
> +	   ((unsigned int) length));
> +}

Small style issue - the GDB coding style requires that we have
an empty line after local variable declarations. So can you add
an empty line after "int length = ....", please?

More importantly, why is length an int when TYPE_LENGTH (type)
is an unsigned int? Similarly to be fore, it then opens the door
for having to check for length being negative which does not make
sense, and it also then forces a cast when using it to print
the type's length.

In terms of the function's behavior, I would add a couple of things
to the error message:
  1. Tell the user that he can increase the maximum using
     the "set max-value-size SIZE" command;
  2. print the type's name, if avaiable, in the error message,
     for easier tracking of the issue.

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

smll formatting nit, binary operators should be at the start of
the next line, rather than the end of the previous one. For the
assignement, this gives us:

      val->contents
        = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));

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

Same here.

-- 
Joel