From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99434 invoked by alias); 28 Jan 2016 15:11:18 -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 99417 invoked by uid 89); 28 Jan 2016 15:11:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=menus, controls, @item, Usage X-HELO: mail-wm0-f54.google.com Received: from mail-wm0-f54.google.com (HELO mail-wm0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 28 Jan 2016 15:11:09 +0000 Received: by mail-wm0-f54.google.com with SMTP id r129so29140927wmr.0 for ; Thu, 28 Jan 2016 07:11:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=VD8Zuh576UUwKN5fSbIpV75ZAx/nVwX3SVjXpwJFnX8=; b=kbcXLx8ug/wPC9vNUCTh29svwIOvhq2TBG1Wv8e05PjAEQVIJo752a3oT5cLNV82Cm XRq5FAJqk/JWD2dUqOctE5pRrjCkNHf0ynQPUPjEnqLolCg7G0Rb9Na+Xv89Qci4b1Tw 9T96jz5Rb3FeKd3mRJ8U3B65BN81FkQ1Ert+q6yNqGAkuNIqBkV4MTBYjeHZssxaSZ0C +eb7rcnsjDapxY0HSCcJhyrl5Kg37seGoV5hYeww0GVyyKGPee+DQTzBRjdZVrxF4+Os KKwAS56cWudKTlJ92km1XLOQOeGmk1ToSkhvMXbjxrKavzzlYPmuYZX9+tpqgIM3pwEk 3Q2A== X-Gm-Message-State: AG10YORFHKndqF9HAqxs30hkS+ZIZ+tFTP7Hon3izJ7XKwj1tZYOw4KcpuVcA2w35XHJvw== X-Received: by 10.194.249.69 with SMTP id ys5mr3475854wjc.97.1453993866525; Thu, 28 Jan 2016 07:11:06 -0800 (PST) Received: from localhost (host86-138-95-125.range86-138.btcentralplus.com. [86.138.95.125]) by smtp.gmail.com with ESMTPSA id jo6sm11457129wjb.48.2016.01.28.07.11.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 28 Jan 2016 07:11:05 -0800 (PST) Date: Thu, 28 Jan 2016 15:11:00 -0000 From: Andrew Burgess To: gdb-patches@sourceware.org Cc: brobecker@adacore.com, Pedro Alves Subject: PING #2: Re: [PATCH 1/3] gdb: New set/show max-value-size command. Message-ID: <20160128151045.GQ3338@embecosm.com> References: <57e2731e179d11c584e8cde994ab1e822a9893b0.1449869722.git.andrew.burgess@embecosm.com> <20160101094309.GC12416@adacore.com> <20160105141241.GG4242@embecosm.com> <83a8ok570f.fsf@gnu.org> <20160106114049.GJ4242@embecosm.com> <20160120105921.GW4242@embecosm.com> <20160120152257.GA7299@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160120152257.GA7299@embecosm.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00704.txt.bz2 * Andrew Burgess [2016-01-20 16:22:57 +0100]: > * Andrew Burgess [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 > + > + * 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 > > * 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 > + > + * gdb.texinfo (Value Sizes): New section. > + (Data): Add the 'Value Sizes' node to the menu. > + > 2016-01-19 John Baldwin > > * 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 > + > + * 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 > > * 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 . */ > + > +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 . > + > +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 \\}" > + } > + 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