Re: [PATCH 4/4] gdb: check max-value-size when reading strings for printf

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

Eli,

Did you have any docs feedback?

Thanks,
Andrew


> Tom suggested this change should have a NEWS entry, so I added one.
> There's no changes to the rest of the patch.
>
> Thanks,
> Andrew
>
> ---
>
> commit b7206ddf6228c44a5bec7f9c3d05747f1ddd5025
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed May 31 21:41:48 2023 +0100
>
>     gdb: check max-value-size when reading strings for printf
>     
>     I noticed that the printf code for strings, printf_c_string and
>     printf_wide_c_string, don't take max-value-size into account, but do
>     load a complete string from the inferior into a GDB buffer.
>     
>     As such it would be possible for an badly behaved inferior to cause
>     GDB to try and allocate an excessively large buffer, potentially
>     crashing GDB, or at least causing GDB to swap lots, which isn't
>     great.
>     
>     We already have a setting to protect against this sort of thing, the
>     'max-value-size'.  So this commit updates the two function mentioned
>     above to check the max-value-size and give an error if the
>     max-value-size is exceeded.
>     
>     If the max-value-size is exceeded, I chose to continue reading
>     inferior memory to figure out how long the string actually is, we just
>     don't store the results.  The benefit of this is that when we give the
>     user an error we can tell the user how big the string actually is,
>     which would allow them to correctly adjust max-value-size, if that's
>     what they choose to do.
>     
>     The default for max-value-size is 64k so there should be no user
>     visible changes after this commit, unless the user was previously
>     printing very large strings.  If that is the case then the user will
>     now need to increase max-value-size.
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 649a3a9824a..82fe2a73fa2 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -78,6 +78,12 @@
>    functionality is also available for dprintf when dprintf-style is
>    'gdb'.
>  
> +* When the printf command requires a string to be fetched from the
> +  inferior, GDB now uses the existing 'max-value-size' setting to the
> +  limit the memory allocated within GDB.  The default 'max-value-size'
> +  is 64k.  To print longer strings you should increase
> +  'max-value-size'.
> +
>  * New commands
>  
>  maintenance print record-instruction [ N ]
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 61b009fb7f2..4bfdaa2c7d7 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2480,17 +2480,24 @@ printf_c_string (struct ui_file *stream, const char *format,
>        /* This is a %s argument.  Build the string in STR which is
>  	 currently empty.  */
>        gdb_assert (str.size () == 0);
> -      for (size_t len = 0;; len++)
> +      size_t len;
> +      for (len = 0;; len++)
>  	{
>  	  gdb_byte c;
>  
>  	  QUIT;
> +
>  	  read_memory (tem + len, &c, 1);
> -	  str.push_back (c);
> +	  if (!exceeds_max_value_size (len + 1))
> +	    str.push_back (c);
>  	  if (c == 0)
>  	    break;
>  	}
>  
> +      if (exceeds_max_value_size (len + 1))
> +	error (_("printed string requires %s bytes, which is more than "
> +		 "max-value-size"), plongest (len + 1));
> +
>        /* We will have passed through the above loop at least once, and will
>  	 only exit the loop when we have pushed a zero byte onto the end of
>  	 STR.  */
> @@ -2547,13 +2554,37 @@ printf_wide_c_string (struct ui_file *stream, const char *format,
>        for (len = 0;; len += wcwidth)
>  	{
>  	  QUIT;
> -	  tem_str->resize (tem_str->size () + wcwidth);
> -	  gdb_byte *dst = tem_str->data () + len;
> +	  gdb_byte *dst;
> +	  if (!exceeds_max_value_size (len + wcwidth))
> +	    {
> +	      tem_str->resize (tem_str->size () + wcwidth);
> +	      dst = tem_str->data () + len;
> +	    }
> +	  else
> +	    {
> +	      /* We still need to check for the null-character, so we need
> +		 somewhere to place the data read from the inferior.  We
> +		 can't keep growing TEM_STR, it's gotten too big, so
> +		 instead just read the new character into the start of
> +		 TEMS_STR.  This will corrupt the previously read contents,
> +		 but we're not going to print this string anyway, we just
> +		 want to know how big it would have been so we can tell the
> +		 user in the error message (see below).
> +
> +		 And we know there will be space in this buffer so long as
> +		 WCWIDTH is smaller than our LONGEST type, the
> +		 max-value-size can't be smaller than a LONGEST.  */
> +	      dst = tem_str->data ();
> +	    }
>  	  read_memory (tem + len, dst, wcwidth);
>  	  if (extract_unsigned_integer (dst, wcwidth, byte_order) == 0)
>  	    break;
>  	}
>  
> +      if (exceeds_max_value_size (len + wcwidth))
> +	error (_("printed string requires %s bytes, which is more than "
> +		 "max-value-size"), plongest (len + wcwidth));
> +
>        str = tem_str->data ();
>      }
>  
> diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
> index fa3a62d6cdd..8445fcc1aa2 100644
> --- a/gdb/testsuite/gdb.base/printcmds.c
> +++ b/gdb/testsuite/gdb.base/printcmds.c
> @@ -75,6 +75,8 @@ char *teststring = (char*)"teststring contents";
>  typedef char *charptr;
>  charptr teststring2 = "more contents";
>  
> +const char *teststring3 = "this is a longer test string that we can use";
> +
>  /* Test printing of a struct containing character arrays. */
>  
>  struct some_arrays {
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 73f145c9586..eab0264af63 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -901,6 +901,11 @@ proc test_printf {} {
>  
>      # PR cli/14977.
>      gdb_test "printf \"%s\\n\", 0" "\\(null\\)"
> +
> +    with_max_value_size 20 {
> +	gdb_test {printf "%s", teststring3} \
> +	    "^printed string requires 45 bytes, which is more than max-value-size"
> +    }
>  }
>  
>  #Test printing DFP values with printf
> diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.c b/gdb/testsuite/gdb.base/printf-wchar_t.c
> index 4d13fd3c961..2635932c780 100644
> --- a/gdb/testsuite/gdb.base/printf-wchar_t.c
> +++ b/gdb/testsuite/gdb.base/printf-wchar_t.c
> @@ -18,6 +18,8 @@
>  #include <wchar.h>
>  
>  const wchar_t wide_str[] = L"wide string";
> +const wchar_t long_wide_str[]
> +  = L"this is a much longer wide string that we can use if needed";
>  
>  int
>  main (void)
> diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.exp b/gdb/testsuite/gdb.base/printf-wchar_t.exp
> index 85c6edf292c..8a6003d5785 100644
> --- a/gdb/testsuite/gdb.base/printf-wchar_t.exp
> +++ b/gdb/testsuite/gdb.base/printf-wchar_t.exp
> @@ -24,3 +24,9 @@ if {![runto_main]} {
>  }
>  
>  gdb_test {printf "%ls\n", wide_str} "^wide string"
> +
> +# Check that if the max-value-size will kick in when using printf on strings.
> +with_max_value_size 20 {
> +    gdb_test {printf "%ls\n", long_wide_str} \
> +	"^printed string requires 240 bytes, which is more than max-value-size"
> +}
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 62e0b0b3db5..321d9707cd5 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3192,6 +3192,36 @@ proc with_target_charset { target_charset body } {
>      }
>  }
>  
> +# Run tests in BODY with max-value-size set to SIZE.  When BODY is
> +# finished restore max-value-size.
> +
> +proc with_max_value_size { size body } {
> +    global gdb_prompt
> +
> +    set saved ""
> +    gdb_test_multiple "show max-value-size" "" {
> +	-re -wrap "Maximum value size is ($::decimal) bytes\\." {
> +	    set saved $expect_out(1,string)
> +	}
> +	-re ".*$gdb_prompt " {
> +	    fail "get max-value-size"
> +	}
> +    }
> +
> +    gdb_test_no_output -nopass "set max-value-size $size"
> +
> +    set code [catch {uplevel 1 $body} result]
> +
> +    gdb_test_no_output -nopass "set max-value-size $saved"
> +
> +    if {$code == 1} {
> +	global errorInfo errorCode
> +	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
> +    } else {
> +	return -code $code $result
> +    }
> +}
> +
>  # Switch the default spawn id to SPAWN_ID, so that gdb_test,
>  # mi_gdb_test etc. default to using it.
>  
> diff --git a/gdb/value.c b/gdb/value.c
> index 980722a6dd7..05e5d10ab38 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -804,7 +804,7 @@ check_type_length_before_alloc (const struct type *type)
>  {
>    ULONGEST length = type->length ();
>  
> -  if (max_value_size > -1 && length > max_value_size)
> +  if (exceeds_max_value_size (length))
>      {
>        if (type->name () != NULL)
>  	error (_("value of type `%s' requires %s bytes, which is more "
> @@ -815,6 +815,14 @@ check_type_length_before_alloc (const struct type *type)
>      }
>  }
>  
> +/* See value.h.  */
> +
> +bool
> +exceeds_max_value_size (ULONGEST length)
> +{
> +  return max_value_size > -1 && length > max_value_size;
> +}
> +
>  /* When this has a value, it is used to limit the number of array elements
>     of an array that are loaded into memory when an array value is made
>     non-lazy.  */
> diff --git a/gdb/value.h b/gdb/value.h
> index 508367a4159..cca356a93ea 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -1575,6 +1575,11 @@ extern void finalize_values ();
>     of floating-point, fixed-point, or integer type.  */
>  extern gdb_mpq value_to_gdb_mpq (struct value *value);
>  
> +/* Return true if LEN (in bytes) exceeds the max-value-size setting,
> +   otherwise, return false.  If the user has disabled (set to unlimited)
> +   the max-value-size setting then this function will always return false.  */
> +extern bool exceeds_max_value_size (ULONGEST length);
> +
>  /* While an instance of this class is live, and array values that are
>     created, that are larger than max_value_size, will be restricted in size
>     to a particular number of elements.  */