[4/4] 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.
---
 gdb/printcmd.c                            | 39 ++++++++++++++++++++---
 gdb/testsuite/gdb.base/printcmds.c        |  2 ++
 gdb/testsuite/gdb.base/printcmds.exp      |  5 +++
 gdb/testsuite/gdb.base/printf-wchar_t.c   |  2 ++
 gdb/testsuite/gdb.base/printf-wchar_t.exp |  6 ++++
 gdb/testsuite/lib/gdb.exp                 | 30 +++++++++++++++++
 gdb/value.c                               | 10 +++++-
 gdb/value.h                               |  5 +++
 8 files changed, 94 insertions(+), 5 deletions(-)
  

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> We already have a setting to protect against this sort of thing, the
Andrew> 'max-value-size'.  So this commit updates the two function mentioned
Andrew> above to check the max-value-size and give an error if the
Andrew> max-value-size is exceeded.

I'm happy with this change, but I wonder if it's user-visible enough to
warrant a NEWS entry.

Tom
  

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

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

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Simon Marchi <simark@simark.ca>, Tom Tromey <tom@tromey.com>, Eli
>  Zaretskii <eliz@gnu.org>
> Date: Tue, 04 Jul 2023 14:20:40 +0100
> 
> 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.

The NEWS entry is OK, thanks.