From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57901 invoked by alias); 1 Jan 2016 09:43:23 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 57892 invoked by uid 89); 1 Jan 2016 09:43:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=Style, importantly, increasing, activate X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 01 Jan 2016 09:43:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 09C40116652; Fri, 1 Jan 2016 04:43:19 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 253djLJfkilR; Fri, 1 Jan 2016 04:43:18 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7F1A7116648; Fri, 1 Jan 2016 04:43:18 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9337243C4D; Fri, 1 Jan 2016 13:43:09 +0400 (RET) Date: Fri, 01 Jan 2016 09:43:00 -0000 From: Joel Brobecker To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] gdb: New set/show max-value-size command. Message-ID: <20160101094309.GC12416@adacore.com> References: <57e2731e179d11c584e8cde994ab1e822a9893b0.1449869722.git.andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57e2731e179d11c584e8cde994ab1e822a9893b0.1449869722.git.andrew.burgess@embecosm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2016-01/txt/msg00003.txt.bz2 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 \\}" > +} > + > +# 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 \\}" > +} > + > +# 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 \\}" > +} 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