[PATCH 0/4] Some alloca removal and a printf bug fix

[PATCH 0/4] Some alloca removal and a printf bug fix

[Andrew Burgess]

I previously posted this series in an attempt to remove lots of alloca use:

  https://inbox.sourceware.org/gdb-patches/cover.1677533215.git.aburgess@redhat.com/

there was some push back against that complete series, however, I
think the problem that was pointed out doesn't apply to one of the
original patches, so this series started with me trying to upstream
that one patch (this is patch #2 in this series).

However, while reviewing the patch again I spotted a bug I'd
introduced.  Which means we have a gap in our testing, as the bug was
not exposed during testing.  So I started to write a test, and hit
another bug (not one introduced by me), this is fixed in patch #1 in
this series.

My original series didn't remove all the uses of alloca from
printcmd.c, I don't recall why.  But on review it's actually pretty
easy to remove the final alloca from printcmd.c, so that's what
patch #3 in this series does.

And finally, while working on patch #2 I realised that a badly formed
inferior (e.g. one that needed debugging due to memory corruption)
could cause GDB to try and allocate a huge ammount of memory,
potentially crashing GDB.  This is mitigated in patch #4.

---

Andrew Burgess (4):
  gdb: fix printf of wchar_t early in a gdb session
  gdb: remove two uses of alloca from printcmd.c
  gdb: remove last alloca call from printcmd.c
  gdb: check max-value-size when reading strings for printf

 gdb/c-lang.c                              |   3 -
 gdb/gdbtypes.c                            |  11 ++-
 gdb/gdbtypes.h                            |  14 ++-
 gdb/printcmd.c                            | 102 +++++++++++++---------
 gdb/testsuite/gdb.base/printcmds.c        |   2 +
 gdb/testsuite/gdb.base/printcmds.exp      |   5 ++
 gdb/testsuite/gdb.base/printf-wchar_t.c   |  28 ++++++
 gdb/testsuite/gdb.base/printf-wchar_t.exp |  32 +++++++
 gdb/testsuite/lib/gdb.exp                 |  30 +++++++
 gdb/value.c                               |  10 ++-
 gdb/value.h                               |   5 ++
 11 files changed, 192 insertions(+), 50 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.c
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.exp


base-commit: e9683acf5e51c2bac8aa68d30d9ac3683dddcc7d
-- 
2.25.4

[PATCH 1/4] gdb: fix printf of wchar_t early in a gdb session

[Andrew Burgess]

Given this test program:

  #include <wchar.h>

  const wchar_t wide_str[] = L"wide string";

  int
  main (void)
  {
    return 0;
  }

I observed this GDB behaviour:

  $ gdb -q /tmp/printf-wchar_t
  Reading symbols from /tmp/printf-wchar_t...
  (gdb) start
  Temporary breakpoint 1 at 0x40110a: file /tmp/printf-wchar_t.c, line 8.
  Starting program: /tmp/printf-wchar_t

  Temporary breakpoint 1, main () at /tmp/printf-wchar_t.c:8
  25	  return 0;
  (gdb) printf "%ls\n", wide_str

  (gdb)

Notice that the printf results in a blank line rather than the
expected 'wide string' output.

I tracked the problem down to printf_wide_c_string (in printcmd.c), in
this function we do this:

  struct type *wctype = lookup_typename (current_language,
					 "wchar_t", NULL, 0);
  int wcwidth = wctype->length ();

the problem here is that 'wchar_t' is a typedef.  If we look at the
comment on type::length() we see this:

  /* Note that if thistype is a TYPEDEF type, you have to call check_typedef.
     But check_typedef does set the TYPE_LENGTH of the TYPEDEF type,
     so you only have to call check_typedef once.  Since value::allocate
     calls check_typedef, X->type ()->length () is safe.  */

What this means is that after calling lookup_typename we should call
check_typedef in order to ensure that the length of the typedef has
been setup correctly.  We are not doing this in printf_wide_c_string,
and so wcwidth is incorrectly calculated as 0.  This is what leads GDB
to print an empty string.

We can see in c_string_operation::evaluate (in c-lang.c) an example of
calling check_typedef specifically to fix this exact issue.

Initially I did fix this problem by adding a check_typedef call into
printf_wide_c_string, but then I figured why not move the
check_typedef call up into lookup_typename itself, that feels like it
should be harmless when looking up a non-typedef type, but will avoid
bugs like this when looking up a typedef.  So that's what I did.

I can then remove the extra check_typedef call from c-lang.c, I don't
see any other places where we had extra check_typedef calls.  This
doesn't mean we definitely had bugs -- so long as we never checked the
length, or, if we knew that check_typedef had already been called,
then we would be fine.

I don't see any test regressions after this change, and my new test
case is now passing.
---
 gdb/c-lang.c                              |  3 ---
 gdb/gdbtypes.c                            | 11 ++++++----
 gdb/gdbtypes.h                            | 14 ++++++++++--
 gdb/testsuite/gdb.base/printf-wchar_t.c   | 26 +++++++++++++++++++++++
 gdb/testsuite/gdb.base/printf-wchar_t.exp | 26 +++++++++++++++++++++++
 5 files changed, 71 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.c
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.exp

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 484f81e455b..677e76ee71b 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -615,9 +615,6 @@ c_string_operation::evaluate (struct type *expect_type,
       internal_error (_("unhandled c_string_type"));
     }
 
-  /* Ensure TYPE_LENGTH is valid for TYPE.  */
-  check_typedef (type);
-
   /* If the caller expects an array of some integral type,
      satisfy them.  If something odder is expected, rely on the
      caller to cast.  */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 10e585066f7..9eecf357152 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1648,9 +1648,7 @@ type_name_or_error (struct type *type)
 	 objfile ? objfile_name (objfile) : "<arch>");
 }
 
-/* Lookup a typedef or primitive type named NAME, visible in lexical
-   block BLOCK.  If NOERR is nonzero, return zero if NAME is not
-   suitably defined.  */
+/* See gdbtypes.h.  */
 
 struct type *
 lookup_typename (const struct language_defn *language,
@@ -1662,7 +1660,12 @@ lookup_typename (const struct language_defn *language,
   sym = lookup_symbol_in_language (name, block, VAR_DOMAIN,
 				   language->la_language, NULL).symbol;
   if (sym != NULL && sym->aclass () == LOC_TYPEDEF)
-    return sym->type ();
+    {
+      struct type *type = sym->type ();
+      /* Ensure the length of TYPE is valid.  */
+      check_typedef (type);
+      return type;
+    }
 
   if (noerr)
     return NULL;
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 319a7731bca..f56de4544b5 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2586,8 +2586,18 @@ extern void check_stub_method_group (struct type *, int);
 
 extern char *gdb_mangle_name (struct type *, int, int);
 
-extern struct type *lookup_typename (const struct language_defn *,
-				     const char *, const struct block *, int);
+/* Lookup a typedef or primitive type named NAME, visible in lexical block
+   BLOCK.  If NOERR is nonzero, return zero if NAME is not suitably
+   defined.
+
+   If this function finds a suitable type then check_typedef is called on
+   the type, this ensures that if the type being returned is a typedef
+   then the length of the type will be correct.  The original typedef will
+   still be returned, not the result of calling check_typedef.  */
+
+extern struct type *lookup_typename (const struct language_defn *language,
+				     const char *name,
+				     const struct block *block, int noerr);
 
 extern struct type *lookup_template_type (const char *, struct type *,
 					  const struct block *);
diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.c b/gdb/testsuite/gdb.base/printf-wchar_t.c
new file mode 100644
index 00000000000..4d13fd3c961
--- /dev/null
+++ b/gdb/testsuite/gdb.base/printf-wchar_t.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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 <http://www.gnu.org/licenses/>.  */
+
+#include <wchar.h>
+
+const wchar_t wide_str[] = L"wide string";
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.exp b/gdb/testsuite/gdb.base/printf-wchar_t.exp
new file mode 100644
index 00000000000..85c6edf292c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/printf-wchar_t.exp
@@ -0,0 +1,26 @@
+# Copyright 2023 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 <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test {printf "%ls\n", wide_str} "^wide string"
-- 
2.25.4

Re: [PATCH 1/4] gdb: fix printf of wchar_t early in a gdb session

[Tom Tromey]

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

> Initially I did fix this problem by adding a check_typedef call into
> printf_wide_c_string, but then I figured why not move the
> check_typedef call up into lookup_typename itself, that feels like it
> should be harmless when looking up a non-typedef type, but will avoid
> bugs like this when looking up a typedef.  So that's what I did.

Makes sense to me.

It's pretty typical for a missing check_typedef to wind up being a bug
in some scenario.  I think one of the dreams for method-izing struct
type is to make this impossible -- like having the various methods call
check_typedef themselves (if needed) before returning an answer.

Anyway, this looks good to me.
Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

[PATCH 2/4] gdb: remove two uses of alloca from printcmd.c

[Andrew Burgess]

Remove a couple of uses of alloca from printcmd.c, and replace them
with gdb::byte_vector.

An earlier variant of this patch was proposed in this thread:

  https://inbox.sourceware.org/gdb-patches/cover.1677533215.git.aburgess@redhat.com/

however, there was push back on that thread due to it adding extra
dynamic allocation, i.e. moving the memory buffers off the stack on to
the heap.

However, of all the patches originally proposed, I think in these two
cases moving off the stack is the correct thing to do.  Unlike all the
other patches in the original series, where the data being read
was (mostly) small in size, a register, or a couple of registers, in
this case we are reading an arbitrary string from the inferior.  This
could be any size, and so should not be placed on the stack.

So in this commit I replace the use of alloca with std::byte_vector
and simplify the logic a little (I think) to take advantage of the
ability of std::byte_vector to dynamically grow in size.

Of course, really, we should probably be checking the max-value-size
setting as we load the string to stop GDB crashing if a corrupted
inferior causes GDB to try read a stupidly large amount of
memory... but I'm leaving that for a follow on patch.

There should be no user visible changes after this commit.
---
 gdb/printcmd.c | 49 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index f9517e6e086..6f8a7f1420a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2447,7 +2447,7 @@ static void
 printf_c_string (struct ui_file *stream, const char *format,
 		 struct value *value)
 {
-  const gdb_byte *str;
+  gdb::byte_vector str;
 
   if (((value->type ()->code () != TYPE_CODE_PTR && value->lval () == lval_internalvar)
        || value->type ()->code () == TYPE_CODE_ARRAY)
@@ -2459,11 +2459,10 @@ printf_c_string (struct ui_file *stream, const char *format,
 	 character.  This protects against corrupted C-style strings that lack
 	 the terminating null char.  It also allows Ada-style strings (not
 	 null terminated) to be printed without problems.  */
-      gdb_byte *tem_str = (gdb_byte *) alloca (len + 1);
+      str.resize (len + 1);
 
-      memcpy (tem_str, value->contents ().data (), len);
-      tem_str [len] = 0;
-      str = tem_str;
+      memcpy (str.data (), value->contents ().data (), len);
+      str [len] = 0;
     }
   else
     {
@@ -2478,31 +2477,30 @@ printf_c_string (struct ui_file *stream, const char *format,
 	  return;
 	}
 
-      /* This is a %s argument.  Find the length of the string.  */
-      size_t len;
-
-      for (len = 0;; len++)
+      /* 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++)
 	{
 	  gdb_byte c;
 
 	  QUIT;
 	  read_memory (tem + len, &c, 1);
+	  str.push_back (c);
 	  if (c == 0)
 	    break;
 	}
 
-      /* Copy the string contents into a string inside GDB.  */
-      gdb_byte *tem_str = (gdb_byte *) alloca (len + 1);
-
-      if (len != 0)
-	read_memory (tem, tem_str, len);
-      tem_str[len] = 0;
-      str = tem_str;
+      /* 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.  */
+      gdb_assert (str.size () > 0);
+      gdb_assert (str.back () == 0);
     }
 
   DIAGNOSTIC_PUSH
   DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
-    gdb_printf (stream, format, (char *) str);
+    gdb_printf (stream, format, (char *) str.data ());
   DIAGNOSTIC_POP
 }
 
@@ -2521,6 +2519,7 @@ printf_wide_c_string (struct ui_file *stream, const char *format,
   struct type *wctype = lookup_typename (current_language,
 					 "wchar_t", NULL, 0);
   int wcwidth = wctype->length ();
+  gdb::optional<gdb::byte_vector> tem_str;
 
   if (value->lval () == lval_internalvar
       && c_is_string_type_p (value->type ()))
@@ -2543,23 +2542,19 @@ printf_wide_c_string (struct ui_file *stream, const char *format,
 
       /* This is a %s argument.  Find the length of the string.  */
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte *buf = (gdb_byte *) alloca (wcwidth);
+      tem_str.emplace ();
 
       for (len = 0;; len += wcwidth)
 	{
 	  QUIT;
-	  read_memory (tem + len, buf, wcwidth);
-	  if (extract_unsigned_integer (buf, wcwidth, byte_order) == 0)
+	  tem_str->resize (tem_str->size () + wcwidth);
+	  gdb_byte *dst = tem_str->data () + len;
+	  read_memory (tem + len, dst, wcwidth);
+	  if (extract_unsigned_integer (dst, wcwidth, byte_order) == 0)
 	    break;
 	}
 
-      /* Copy the string contents into a string inside GDB.  */
-      gdb_byte *tem_str = (gdb_byte *) alloca (len + wcwidth);
-
-      if (len != 0)
-	read_memory (tem, tem_str, len);
-      memset (&tem_str[len], 0, wcwidth);
-      str = tem_str;
+      str = tem_str->data ();
     }
 
   auto_obstack output;
-- 
2.25.4

[PATCH 3/4] gdb: remove last alloca call from printcmd.c

[Andrew Burgess]

This commit removes the last alloca call from printcmd.c.  This is
similar to the patches I originally posted here:

  https://inbox.sourceware.org/gdb-patches/cover.1677533215.git.aburgess@redhat.com/

However, this change was not included in that original series.

The original series received push back because it was thought that
replacing alloca with a C++ container type would introduce unnecessary
malloc/free overhead.

However, in this case we are building a string, and (at least for
GCC), the std::string type has a small string optimisation, where
small strings are stored on the stack.

And in this case we are building what will usually be a very small
string, we're just constructing a printf format specifier for a hex
value, so it'll be something like '%#x' -- though it could also have a
width in there too -- but still, it should normally fit within GCCs
small string buffer.

So, in this commit, I propose replacing the use of alloca with a
std::string.  This shouldn't result (normally) in any additional
malloc or free calls, so should be similar in performance to the
original approach.

There should be no user visible differences after this commit.
---
 gdb/printcmd.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 6f8a7f1420a..61b009fb7f2 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2653,62 +2653,58 @@ printf_pointer (struct ui_file *stream, const char *format,
      modifier for %p is a width; extract that, and then
      handle %p as glibc would: %#x or a literal "(nil)".  */
 
-  const char *p;
-  char *fmt, *fmt_p;
 #ifdef PRINTF_HAS_LONG_LONG
   long long val = value_as_long (value);
 #else
   long val = value_as_long (value);
 #endif
 
-  fmt = (char *) alloca (strlen (format) + 5);
+  /* Build the new output format in FMT.  */
+  std::string fmt;
 
   /* Copy up to the leading %.  */
-  p = format;
-  fmt_p = fmt;
+  const char *p = format;
   while (*p)
     {
       int is_percent = (*p == '%');
 
-      *fmt_p++ = *p++;
+      fmt.push_back (*p++);
       if (is_percent)
 	{
 	  if (*p == '%')
-	    *fmt_p++ = *p++;
+	    fmt.push_back (*p++);
 	  else
 	    break;
 	}
     }
 
   if (val != 0)
-    *fmt_p++ = '#';
+    fmt.push_back ('#');
 
   /* Copy any width or flags.  Only the "-" flag is valid for pointers
      -- see the format_pieces constructor.  */
   while (*p == '-' || (*p >= '0' && *p < '9'))
-    *fmt_p++ = *p++;
+    fmt.push_back (*p++);
 
   gdb_assert (*p == 'p' && *(p + 1) == '\0');
   if (val != 0)
     {
 #ifdef PRINTF_HAS_LONG_LONG
-      *fmt_p++ = 'l';
+      fmt.push_back ('l');
 #endif
-      *fmt_p++ = 'l';
-      *fmt_p++ = 'x';
-      *fmt_p++ = '\0';
+      fmt.push_back ('l');
+      fmt.push_back ('x');
       DIAGNOSTIC_PUSH
       DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
-	gdb_printf (stream, fmt, val);
+	gdb_printf (stream, fmt.c_str (), val);
       DIAGNOSTIC_POP
     }
   else
     {
-      *fmt_p++ = 's';
-      *fmt_p++ = '\0';
+      fmt.push_back ('s');
       DIAGNOSTIC_PUSH
       DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
-	gdb_printf (stream, fmt, "(nil)");
+	gdb_printf (stream, fmt.c_str (), "(nil)");
       DIAGNOSTIC_POP
     }
 }
-- 
2.25.4

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

[Andrew Burgess]

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

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 294d136a547..c8d0aa6d3b8 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.  */
-- 
2.25.4

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

[Tom Tromey]

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

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

[Andrew Burgess]

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

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

[Andrew Burgess]

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

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

[Eli Zaretskii]

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

Re: [PATCH 0/4] Some alloca removal and a printf bug fix

[Tom Tromey]

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

Andrew> there was some push back against that complete series, however, I
Andrew> think the problem that was pointed out doesn't apply to one of the
Andrew> original patches, so this series started with me trying to upstream
Andrew> that one patch (this is patch #2 in this series).

FWIW I read through these and I think they are fine.

Tom

Re: [PATCH 0/4] Some alloca removal and a printf bug fix

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> there was some push back against that complete series, however, I
> Andrew> think the problem that was pointed out doesn't apply to one of the
> Andrew> original patches, so this series started with me trying to upstream
> Andrew> that one patch (this is patch #2 in this series).
>
> FWIW I read through these and I think they are fine.

I pushed this series.

Thanks,
Andrew