[PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

[PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

[Luis Machado]

remote_fileio_extract_ptr_w_len parsed a debuggee-controlled 64-bit
length, narrowed it to int with no bounds check, and the value flowed
directly into alloca() across RSP File-I/O handlers (Fopen, Frename,
Funlink, Fstat, Fsystem).  A malicious inferior or remote
stub could send a crafted packet with a path-length near INT_MAX,
displacing the stack pointer by ~2 GiB and potentially crashing the debugger.

remote_fileio_extract_ptr_w_len now rejects negative lengths, zero, and
values above PATH_MAX before the int cast.  An allow_zero_length flag
lets the Fsystem handler opt in to the zero-length sentinel that signals a
NULL cmdline query.  All other handlers use the default and are protected
without any per-call checks.  All alloca(length) calls have been replaced
with gdb::char_vector.

Add a selftest that exercises various problematic cases.

Signed-off-by: Luis Machado <luis.machado@amd.com>
---
 gdb/remote-fileio.c | 150 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 119 insertions(+), 31 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index ef608cabfab..50c7869a1a5 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -31,8 +31,11 @@
 #include "target.h"
 #include "filenames.h"
 #include "gdbsupport/filestuff.h"
+#include "gdbsupport/byte-vector.h"
+#include "gdbsupport/selftest.h"
 
 #include <fcntl.h>
+#include <climits>
 #include "gdbsupport/gdb_sys_time.h"
 #ifdef __CYGWIN__
 #include <sys/cygwin.h>
@@ -194,7 +197,8 @@ remote_fileio_extract_int (char **buf, long *retint)
 }
 
 static int
-remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
+remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
+				  bool allow_zero_length = false)
 {
   char *c;
   LONGEST retlong;
@@ -211,6 +215,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
   *buf = c;
   if (remote_fileio_extract_long (buf, &retlong))
     return -1;
+  /* Reject negative lengths, zero (unless the caller permits it for the
+     Fsystem NULL-cmdline sentinel), and lengths above PATH_MAX to prevent
+     out-of-bounds memory reads or oversized heap allocations.  */
+  if (retlong < 0 || (!allow_zero_length && retlong == 0) || retlong > PATH_MAX)
+    return -1;
   *length = (int) retlong;
   return 0;
 }
@@ -305,7 +314,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   long num;
   int flags, fd;
   mode_t mode;
-  char *pathname;
   struct stat st;
 
   /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
@@ -339,8 +347,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
     }
 
   /* Request pathname.  */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::char_vector pathname (length);
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.data (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -349,7 +357,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   /* Check if pathname exists and is not a regular file or directory.  If so,
      return an appropriate error code.  Same for trying to open directories
      for writing.  */
-  if (!stat (pathname, &st))
+  if (!stat (pathname.data (), &st))
     {
       if (!S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
 	{
@@ -364,7 +372,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
 	}
     }
 
-  fd = gdb_open_cloexec (pathname, flags, mode).release ();
+  fd = gdb_open_cloexec (pathname.data (), flags, mode).release ();
   if (fd < 0)
     {
       remote_fileio_return_errno (remote, -1);
@@ -659,7 +667,6 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 {
   CORE_ADDR old_ptr, new_ptr;
   int old_len, new_len;
-  char *oldpath, *newpath;
   int ret, of, nf;
   struct stat ost, nst;
 
@@ -678,24 +685,24 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
     }
 
   /* Request oldpath using 'm' packet */
-  oldpath = (char *) alloca (old_len);
-  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
+  gdb::char_vector oldpath (old_len);
+  if (target_read_memory (old_ptr, (gdb_byte *) oldpath.data (), old_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Request newpath using 'm' packet */
-  newpath = (char *) alloca (new_len);
-  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
+  gdb::char_vector newpath (new_len);
+  if (target_read_memory (new_ptr, (gdb_byte *) newpath.data (), new_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Only operate on regular files and directories.  */
-  of = stat (oldpath, &ost);
-  nf = stat (newpath, &nst);
+  of = stat (oldpath.data (), &ost);
+  nf = stat (newpath.data (), &nst);
   if ((!of && !S_ISREG (ost.st_mode) && !S_ISDIR (ost.st_mode))
       || (!nf && !S_ISREG (nst.st_mode) && !S_ISDIR (nst.st_mode)))
     {
@@ -703,7 +710,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
       return;
     }
 
-  ret = rename (oldpath, newpath);
+  ret = rename (oldpath.data (), newpath.data ());
 
   if (ret == -1)
     {
@@ -726,10 +733,10 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 		  char newfullpath[PATH_MAX];
 		  int len;
 
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath, oldfullpath,
-				    PATH_MAX);
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath, newfullpath,
-				    PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath.data (),
+				    oldfullpath, PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath.data (),
+				    newfullpath, PATH_MAX);
 		  len = strlen (oldfullpath);
 		  if (IS_DIR_SEPARATOR (newfullpath[len])
 		      && !filename_ncmp (oldfullpath, newfullpath, len))
@@ -752,7 +759,6 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int length;
-  char *pathname;
   int ret;
   struct stat st;
 
@@ -763,8 +769,8 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
       return;
     }
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::char_vector pathname (length);
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.data (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -772,13 +778,13 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 
   /* Only operate on regular files (and directories, which allows to return
      the correct return code).  */
-  if (!stat (pathname, &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
+  if (!stat (pathname.data (), &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
     {
       remote_fileio_reply (remote, -1, FILEIO_ENODEV);
       return;
     }
 
-  ret = unlink (pathname);
+  ret = unlink (pathname.data ());
 
   if (ret == -1)
     remote_fileio_return_errno (remote, -1);
@@ -791,7 +797,6 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
 {
   CORE_ADDR statptr, nameptr;
   int ret, namelength;
-  char *pathname;
   LONGEST lnum;
   struct stat st;
   struct fio_stat fst;
@@ -812,14 +817,14 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
   statptr = (CORE_ADDR) lnum;
 
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (namelength);
-  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
+  gdb::char_vector pathname (namelength);
+  if (target_read_memory (nameptr, (gdb_byte *) pathname.data (), namelength) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
-  ret = stat (pathname, &st);
+  ret = stat (pathname.data (), &st);
 
   if (ret == -1)
     {
@@ -997,24 +1002,28 @@ remote_fileio_func_system (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int ret, length;
-  char *cmdline = NULL;
 
   /* Parameter: Ptr to commandline / length incl. trailing zero */
-  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
+  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length, true))
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
+  gdb::char_vector cmdline_buf;
+  const char *cmdline = nullptr;
+
   if (length)
     {
       /* Request commandline using 'm' packet */
-      cmdline = (char *) alloca (length);
-      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
+      cmdline_buf.resize (length);
+      if (target_read_memory (ptrval, (gdb_byte *) cmdline_buf.data (),
+			      length) != 0)
 	{
 	  remote_fileio_ioerror (remote);
 	  return;
 	}
+      cmdline = cmdline_buf.data ();
     }
 
   /* Check if system(3) has been explicitly allowed using the
@@ -1244,6 +1253,81 @@ show_system_call_allowed (const char *args, int from_tty)
 	      remote_fio_system_call_allowed ? "" : "not ");
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Verify that remote_fileio_extract_ptr_w_len rejects lengths that are
+   negative or exceed PATH_MAX, and accepts boundary values.  The packet
+   buffer format is "ptr/len[,...]" with both fields as hex integers.  */
+
+static void
+test_remote_fileio_extract_ptr_w_len ()
+{
+  CORE_ADDR ptrval;
+  int length;
+
+  /* Build a writable "ptr/len" buffer and parse it, with and without
+     the Fsystem zero-length allowance.  */
+  auto parse = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length);
+    };
+  auto parse_allow_zero = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length, true);
+    };
+
+  /* Valid: length 1 (minimum positive).  Verify both fields are parsed.  */
+  SELF_CHECK (parse ("deadbeef/1") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 1);
+
+  /* Valid: packet with trailing fields.  */
+  SELF_CHECK (parse ("deadbeef/4,0,1a4") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 4);
+
+  /* Valid: length 0 with allow_zero_length (Fsystem).  */
+  SELF_CHECK (parse_allow_zero ("0/0") == 0);
+  SELF_CHECK (length == 0);
+
+  /* Invalid: length 0 without allow_zero_length (pathname handlers).  */
+  SELF_CHECK (parse ("0/0") == -1);
+
+  /* Valid: length exactly PATH_MAX.  */
+  {
+    char hex[32];
+    xsnprintf (hex, sizeof (hex), "1/%x", PATH_MAX);
+    SELF_CHECK (parse (hex) == 0);
+    SELF_CHECK (length == PATH_MAX);
+  }
+
+  /* Invalid: length PATH_MAX + 1.  */
+  {
+    char hex[32];
+    xsnprintf (hex, sizeof (hex), "1/%x", PATH_MAX + 1);
+    SELF_CHECK (parse (hex) == -1);
+  }
+
+  /* Invalid: value well above PATH_MAX (INT_MAX as a stress case).  */
+  SELF_CHECK (parse ("1/7fffffff") == -1);
+
+  /* Invalid: negative length.  */
+  SELF_CHECK (parse ("1/-1") == -1);
+
+  /* Invalid: missing '/' separator.  */
+  SELF_CHECK (parse ("deadbeef") == -1);
+}
+
+} /* namespace selftests */
+
+#endif /* GDB_SELF_TEST */
+
 void
 initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 			  struct cmd_list_element **remote_show_cmdlist)
@@ -1256,4 +1340,8 @@ initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 	   show_system_call_allowed,
 	   _("Show if the host system(3) call is allowed for the target."),
 	   remote_show_cmdlist);
+#if GDB_SELF_TEST
+  selftests::register_test ("remote_fileio_extract_ptr_w_len",
+			    selftests::test_remote_fileio_extract_ptr_w_len);
+#endif
 }
-- 
2.34.1

Re: [PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

[Guinevere Larsen]

On 6/30/26 6:52 AM, Luis Machado wrote:
> remote_fileio_extract_ptr_w_len parsed a debuggee-controlled 64-bit
> length, narrowed it to int with no bounds check, and the value flowed
> directly into alloca() across RSP File-I/O handlers (Fopen, Frename,
> Funlink, Fstat, Fsystem).  A malicious inferior or remote
> stub could send a crafted packet with a path-length near INT_MAX,
> displacing the stack pointer by ~2 GiB and potentially crashing the debugger.
>
> remote_fileio_extract_ptr_w_len now rejects negative lengths, zero, and
> values above PATH_MAX before the int cast.  An allow_zero_length flag
> lets the Fsystem handler opt in to the zero-length sentinel that signals a
> NULL cmdline query.  All other handlers use the default and are protected
> without any per-call checks.  All alloca(length) calls have been replaced
> with gdb::char_vector.
>
> Add a selftest that exercises various problematic cases.
>
> Signed-off-by: Luis Machado <luis.machado@amd.com>

Hi Luis!

I took a look at this and everything looks good! And I'm always happy to 
see fewer uses of alloca

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)

> ---
>   gdb/remote-fileio.c | 150 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 119 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
> index ef608cabfab..50c7869a1a5 100644
> --- a/gdb/remote-fileio.c
> +++ b/gdb/remote-fileio.c
> @@ -31,8 +31,11 @@
>   #include "target.h"
>   #include "filenames.h"
>   #include "gdbsupport/filestuff.h"
> +#include "gdbsupport/byte-vector.h"
> +#include "gdbsupport/selftest.h"
>   
>   #include <fcntl.h>
> +#include <climits>
>   #include "gdbsupport/gdb_sys_time.h"
>   #ifdef __CYGWIN__
>   #include <sys/cygwin.h>
> @@ -194,7 +197,8 @@ remote_fileio_extract_int (char **buf, long *retint)
>   }
>   
>   static int
> -remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
> +remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
> +				  bool allow_zero_length = false)
>   {
>     char *c;
>     LONGEST retlong;
> @@ -211,6 +215,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
>     *buf = c;
>     if (remote_fileio_extract_long (buf, &retlong))
>       return -1;
> +  /* Reject negative lengths, zero (unless the caller permits it for the
> +     Fsystem NULL-cmdline sentinel), and lengths above PATH_MAX to prevent
> +     out-of-bounds memory reads or oversized heap allocations.  */
> +  if (retlong < 0 || (!allow_zero_length && retlong == 0) || retlong > PATH_MAX)
> +    return -1;
>     *length = (int) retlong;
>     return 0;
>   }
> @@ -305,7 +314,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>     long num;
>     int flags, fd;
>     mode_t mode;
> -  char *pathname;
>     struct stat st;
>   
>     /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
> @@ -339,8 +347,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>       }
>   
>     /* Request pathname.  */
> -  pathname = (char *) alloca (length);
> -  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
> +  gdb::char_vector pathname (length);
> +  if (target_read_memory (ptrval, (gdb_byte *) pathname.data (), length) != 0)
>       {
>         remote_fileio_ioerror (remote);
>         return;
> @@ -349,7 +357,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>     /* Check if pathname exists and is not a regular file or directory.  If so,
>        return an appropriate error code.  Same for trying to open directories
>        for writing.  */
> -  if (!stat (pathname, &st))
> +  if (!stat (pathname.data (), &st))
>       {
>         if (!S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
>   	{
> @@ -364,7 +372,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>   	}
>       }
>   
> -  fd = gdb_open_cloexec (pathname, flags, mode).release ();
> +  fd = gdb_open_cloexec (pathname.data (), flags, mode).release ();
>     if (fd < 0)
>       {
>         remote_fileio_return_errno (remote, -1);
> @@ -659,7 +667,6 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
>   {
>     CORE_ADDR old_ptr, new_ptr;
>     int old_len, new_len;
> -  char *oldpath, *newpath;
>     int ret, of, nf;
>     struct stat ost, nst;
>   
> @@ -678,24 +685,24 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
>       }
>   
>     /* Request oldpath using 'm' packet */
> -  oldpath = (char *) alloca (old_len);
> -  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
> +  gdb::char_vector oldpath (old_len);
> +  if (target_read_memory (old_ptr, (gdb_byte *) oldpath.data (), old_len) != 0)
>       {
>         remote_fileio_ioerror (remote);
>         return;
>       }
>   
>     /* Request newpath using 'm' packet */
> -  newpath = (char *) alloca (new_len);
> -  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
> +  gdb::char_vector newpath (new_len);
> +  if (target_read_memory (new_ptr, (gdb_byte *) newpath.data (), new_len) != 0)
>       {
>         remote_fileio_ioerror (remote);
>         return;
>       }
>   
>     /* Only operate on regular files and directories.  */
> -  of = stat (oldpath, &ost);
> -  nf = stat (newpath, &nst);
> +  of = stat (oldpath.data (), &ost);
> +  nf = stat (newpath.data (), &nst);
>     if ((!of && !S_ISREG (ost.st_mode) && !S_ISDIR (ost.st_mode))
>         || (!nf && !S_ISREG (nst.st_mode) && !S_ISDIR (nst.st_mode)))
>       {
> @@ -703,7 +710,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
>         return;
>       }
>   
> -  ret = rename (oldpath, newpath);
> +  ret = rename (oldpath.data (), newpath.data ());
>   
>     if (ret == -1)
>       {
> @@ -726,10 +733,10 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
>   		  char newfullpath[PATH_MAX];
>   		  int len;
>   
> -		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath, oldfullpath,
> -				    PATH_MAX);
> -		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath, newfullpath,
> -				    PATH_MAX);
> +		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath.data (),
> +				    oldfullpath, PATH_MAX);
> +		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath.data (),
> +				    newfullpath, PATH_MAX);
>   		  len = strlen (oldfullpath);
>   		  if (IS_DIR_SEPARATOR (newfullpath[len])
>   		      && !filename_ncmp (oldfullpath, newfullpath, len))
> @@ -752,7 +759,6 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
>   {
>     CORE_ADDR ptrval;
>     int length;
> -  char *pathname;
>     int ret;
>     struct stat st;
>   
> @@ -763,8 +769,8 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
>         return;
>       }
>     /* Request pathname using 'm' packet */
> -  pathname = (char *) alloca (length);
> -  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
> +  gdb::char_vector pathname (length);
> +  if (target_read_memory (ptrval, (gdb_byte *) pathname.data (), length) != 0)
>       {
>         remote_fileio_ioerror (remote);
>         return;
> @@ -772,13 +778,13 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
>   
>     /* Only operate on regular files (and directories, which allows to return
>        the correct return code).  */
> -  if (!stat (pathname, &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
> +  if (!stat (pathname.data (), &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
>       {
>         remote_fileio_reply (remote, -1, FILEIO_ENODEV);
>         return;
>       }
>   
> -  ret = unlink (pathname);
> +  ret = unlink (pathname.data ());
>   
>     if (ret == -1)
>       remote_fileio_return_errno (remote, -1);
> @@ -791,7 +797,6 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
>   {
>     CORE_ADDR statptr, nameptr;
>     int ret, namelength;
> -  char *pathname;
>     LONGEST lnum;
>     struct stat st;
>     struct fio_stat fst;
> @@ -812,14 +817,14 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
>     statptr = (CORE_ADDR) lnum;
>   
>     /* Request pathname using 'm' packet */
> -  pathname = (char *) alloca (namelength);
> -  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
> +  gdb::char_vector pathname (namelength);
> +  if (target_read_memory (nameptr, (gdb_byte *) pathname.data (), namelength) != 0)
>       {
>         remote_fileio_ioerror (remote);
>         return;
>       }
>   
> -  ret = stat (pathname, &st);
> +  ret = stat (pathname.data (), &st);
>   
>     if (ret == -1)
>       {
> @@ -997,24 +1002,28 @@ remote_fileio_func_system (remote_target *remote, char *buf)
>   {
>     CORE_ADDR ptrval;
>     int ret, length;
> -  char *cmdline = NULL;
>   
>     /* Parameter: Ptr to commandline / length incl. trailing zero */
> -  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
> +  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length, true))
>       {
>         remote_fileio_ioerror (remote);
>         return;
>       }
>   
> +  gdb::char_vector cmdline_buf;
> +  const char *cmdline = nullptr;
> +
>     if (length)
>       {
>         /* Request commandline using 'm' packet */
> -      cmdline = (char *) alloca (length);
> -      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
> +      cmdline_buf.resize (length);
> +      if (target_read_memory (ptrval, (gdb_byte *) cmdline_buf.data (),
> +			      length) != 0)
>   	{
>   	  remote_fileio_ioerror (remote);
>   	  return;
>   	}
> +      cmdline = cmdline_buf.data ();
>       }
>   
>     /* Check if system(3) has been explicitly allowed using the
> @@ -1244,6 +1253,81 @@ show_system_call_allowed (const char *args, int from_tty)
>   	      remote_fio_system_call_allowed ? "" : "not ");
>   }
>   
> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> +/* Verify that remote_fileio_extract_ptr_w_len rejects lengths that are
> +   negative or exceed PATH_MAX, and accepts boundary values.  The packet
> +   buffer format is "ptr/len[,...]" with both fields as hex integers.  */
> +
> +static void
> +test_remote_fileio_extract_ptr_w_len ()
> +{
> +  CORE_ADDR ptrval;
> +  int length;
> +
> +  /* Build a writable "ptr/len" buffer and parse it, with and without
> +     the Fsystem zero-length allowance.  */
> +  auto parse = [&] (const char *s) -> int
> +    {
> +      std::string buf (s);
> +      char *p = buf.data ();
> +      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length);
> +    };
> +  auto parse_allow_zero = [&] (const char *s) -> int
> +    {
> +      std::string buf (s);
> +      char *p = buf.data ();
> +      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length, true);
> +    };
> +
> +  /* Valid: length 1 (minimum positive).  Verify both fields are parsed.  */
> +  SELF_CHECK (parse ("deadbeef/1") == 0);
> +  SELF_CHECK (ptrval == 0xdeadbeef);
> +  SELF_CHECK (length == 1);
> +
> +  /* Valid: packet with trailing fields.  */
> +  SELF_CHECK (parse ("deadbeef/4,0,1a4") == 0);
> +  SELF_CHECK (ptrval == 0xdeadbeef);
> +  SELF_CHECK (length == 4);
> +
> +  /* Valid: length 0 with allow_zero_length (Fsystem).  */
> +  SELF_CHECK (parse_allow_zero ("0/0") == 0);
> +  SELF_CHECK (length == 0);
> +
> +  /* Invalid: length 0 without allow_zero_length (pathname handlers).  */
> +  SELF_CHECK (parse ("0/0") == -1);
> +
> +  /* Valid: length exactly PATH_MAX.  */
> +  {
> +    char hex[32];
> +    xsnprintf (hex, sizeof (hex), "1/%x", PATH_MAX);
> +    SELF_CHECK (parse (hex) == 0);
> +    SELF_CHECK (length == PATH_MAX);
> +  }
> +
> +  /* Invalid: length PATH_MAX + 1.  */
> +  {
> +    char hex[32];
> +    xsnprintf (hex, sizeof (hex), "1/%x", PATH_MAX + 1);
> +    SELF_CHECK (parse (hex) == -1);
> +  }
> +
> +  /* Invalid: value well above PATH_MAX (INT_MAX as a stress case).  */
> +  SELF_CHECK (parse ("1/7fffffff") == -1);
> +
> +  /* Invalid: negative length.  */
> +  SELF_CHECK (parse ("1/-1") == -1);
> +
> +  /* Invalid: missing '/' separator.  */
> +  SELF_CHECK (parse ("deadbeef") == -1);
> +}
> +
> +} /* namespace selftests */
> +
> +#endif /* GDB_SELF_TEST */
> +
>   void
>   initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
>   			  struct cmd_list_element **remote_show_cmdlist)
> @@ -1256,4 +1340,8 @@ initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
>   	   show_system_call_allowed,
>   	   _("Show if the host system(3) call is allowed for the target."),
>   	   remote_show_cmdlist);
> +#if GDB_SELF_TEST
> +  selftests::register_test ("remote_fileio_extract_ptr_w_len",
> +			    selftests::test_remote_fileio_extract_ptr_w_len);
> +#endif
>   }

Re: [PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

[Pedro Alves]

On 2026-06-30 10:52, Luis Machado wrote:

>  static int
> -remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
> +remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
> +				  bool allow_zero_length = false)
>  {
>    char *c;
>    LONGEST retlong;
> @@ -211,6 +215,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
>    *buf = c;
>    if (remote_fileio_extract_long (buf, &retlong))
>      return -1;
> +  /* Reject negative lengths, zero (unless the caller permits it for the
> +     Fsystem NULL-cmdline sentinel), and lengths above PATH_MAX to prevent
> +     out-of-bounds memory reads or oversized heap allocations.  */
> +  if (retlong < 0 || (!allow_zero_length && retlong == 0) || retlong > PATH_MAX)
> +    return -1;


PATH_MAX is NOT guaranteed to exist.  It's a portability hazard.  Note how the only place in
common code that uses it, in inf-child.c, has "#if defined (PATH_MAX)" guarding it.
The uses in remote-fileio.c itself are guarded by __CYGWIN__.

POSIX allows PATH_MAX to be undefined when the limit is indeterminate.  GNU/Hurd is the canonical case,
it has no PATH_MAX at all.

On Windows, there's MAX_PATH (not a typo, it's really the reserve word order of PATH_MAX), defined as 260,
though syscalls allow more than that.  gnulib's import/pathmax.h does give us PATH_MAX on Windows (but not
on Hurd, see comments there), but also defined as 260.

On macOS PATH_MAX is 1024, but some syscalls accept longer.

Etc.

The GNU conventions is just to not code a limit, and use dynamic allocation.  The patch already switches
from alloca to the heap, so we can just drop the PATH_MAX cap.  The buffers are transient and
passed to open, stat etc. syscalls immediately, and release immediately.  The syscalls fail with ENAMETOOLONG
if truly passed a too-long name, so the cap adds zero safety.


>    *length = (int) retlong;
>    return 0;
>  }
> @@ -305,7 +314,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>    long num;
>    int flags, fd;
>    mode_t mode;
> -  char *pathname;
>    struct stat st;
>  
>    /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
> @@ -339,8 +347,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>      }
>  
>    /* Request pathname.  */
> -  pathname = (char *) alloca (length);
> -  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
> +  gdb::char_vector pathname (length);

"pathname" never needs to be resized, so std::vector adds a bit of useless weight (it needs to
track storage vs size separately, etc.).  Make it a gdb::unique_xmalloc_ptr<char> instead.
That's the only change needed, all the rest of the code will work the same.

(Ditto for all the other instances.  You do have one resize call further below, but that's
not a real resize, it's still just initialization.)

[PATCH v2] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Luis Machado]

remote_fileio_extract_ptr_w_len parsed a debuggee controlled 64 bit
length, narrowed it to int with no bounds check, and the value flowed
directly into alloca() across RSP File I/O handlers (Fopen, Frename,
Funlink, Fstat, Fsystem).  A malicious inferior or remote stub could
send a crafted packet with a large path length, displacing the stack
pointer by up to 2 GiB and potentially crashing the debugger.

remote_fileio_extract_ptr_w_len now rejects negative lengths and zero.
An allow_zero_length flag lets the Fsystem handler opt in to the zero
length sentinel that signals a NULL cmdline query.  All other handlers
use the default and are protected without any per call checks.  Oversized
names are rejected naturally by the underlying syscall with ENAMETOOLONG.
All alloca(length) calls have been replaced with
gdb::unique_xmalloc_ptr<char>, so an oversized but positive length now
fails as a graceful heap allocation error rather than corrupting the
stack.

Add a selftest that exercises various problematic cases.

Signed-off-by: Luis Machado <luis.machado@amd.com>
---
 gdb/remote-fileio.c | 139 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 108 insertions(+), 31 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index ef608cabfab..24d9e1545f9 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -31,6 +31,7 @@
 #include "target.h"
 #include "filenames.h"
 #include "gdbsupport/filestuff.h"
+#include "gdbsupport/selftest.h"
 
 #include <fcntl.h>
 #include "gdbsupport/gdb_sys_time.h"
@@ -194,7 +195,8 @@ remote_fileio_extract_int (char **buf, long *retint)
 }
 
 static int
-remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
+remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
+				  bool allow_zero_length = false)
 {
   char *c;
   LONGEST retlong;
@@ -211,6 +213,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
   *buf = c;
   if (remote_fileio_extract_long (buf, &retlong))
     return -1;
+  /* Reject negative lengths and zero (unless the caller permits it for the
+     Fsystem NULL-cmdline sentinel).  Oversized names are caught by the
+     syscall.  */
+  if (retlong < 0 || (!allow_zero_length && retlong == 0))
+    return -1;
   *length = (int) retlong;
   return 0;
 }
@@ -305,7 +312,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   long num;
   int flags, fd;
   mode_t mode;
-  char *pathname;
   struct stat st;
 
   /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
@@ -339,8 +345,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
     }
 
   /* Request pathname.  */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (length));
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.get (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -349,7 +355,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   /* Check if pathname exists and is not a regular file or directory.  If so,
      return an appropriate error code.  Same for trying to open directories
      for writing.  */
-  if (!stat (pathname, &st))
+  if (!stat (pathname.get (), &st))
     {
       if (!S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
 	{
@@ -364,7 +370,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
 	}
     }
 
-  fd = gdb_open_cloexec (pathname, flags, mode).release ();
+  fd = gdb_open_cloexec (pathname.get (), flags, mode).release ();
   if (fd < 0)
     {
       remote_fileio_return_errno (remote, -1);
@@ -659,7 +665,6 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 {
   CORE_ADDR old_ptr, new_ptr;
   int old_len, new_len;
-  char *oldpath, *newpath;
   int ret, of, nf;
   struct stat ost, nst;
 
@@ -678,24 +683,24 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
     }
 
   /* Request oldpath using 'm' packet */
-  oldpath = (char *) alloca (old_len);
-  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
+  gdb::unique_xmalloc_ptr<char> oldpath ((char *) xmalloc (old_len));
+  if (target_read_memory (old_ptr, (gdb_byte *) oldpath.get (), old_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Request newpath using 'm' packet */
-  newpath = (char *) alloca (new_len);
-  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
+  gdb::unique_xmalloc_ptr<char> newpath ((char *) xmalloc (new_len));
+  if (target_read_memory (new_ptr, (gdb_byte *) newpath.get (), new_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Only operate on regular files and directories.  */
-  of = stat (oldpath, &ost);
-  nf = stat (newpath, &nst);
+  of = stat (oldpath.get (), &ost);
+  nf = stat (newpath.get (), &nst);
   if ((!of && !S_ISREG (ost.st_mode) && !S_ISDIR (ost.st_mode))
       || (!nf && !S_ISREG (nst.st_mode) && !S_ISDIR (nst.st_mode)))
     {
@@ -703,7 +708,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
       return;
     }
 
-  ret = rename (oldpath, newpath);
+  ret = rename (oldpath.get (), newpath.get ());
 
   if (ret == -1)
     {
@@ -726,10 +731,10 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 		  char newfullpath[PATH_MAX];
 		  int len;
 
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath, oldfullpath,
-				    PATH_MAX);
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath, newfullpath,
-				    PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath.get (),
+				    oldfullpath, PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath.get (),
+				    newfullpath, PATH_MAX);
 		  len = strlen (oldfullpath);
 		  if (IS_DIR_SEPARATOR (newfullpath[len])
 		      && !filename_ncmp (oldfullpath, newfullpath, len))
@@ -752,7 +757,6 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int length;
-  char *pathname;
   int ret;
   struct stat st;
 
@@ -763,8 +767,8 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
       return;
     }
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (length));
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.get (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -772,13 +776,14 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 
   /* Only operate on regular files (and directories, which allows to return
      the correct return code).  */
-  if (!stat (pathname, &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
+  if (!stat (pathname.get (), &st) && !S_ISREG (st.st_mode)
+      && !S_ISDIR (st.st_mode))
     {
       remote_fileio_reply (remote, -1, FILEIO_ENODEV);
       return;
     }
 
-  ret = unlink (pathname);
+  ret = unlink (pathname.get ());
 
   if (ret == -1)
     remote_fileio_return_errno (remote, -1);
@@ -791,7 +796,6 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
 {
   CORE_ADDR statptr, nameptr;
   int ret, namelength;
-  char *pathname;
   LONGEST lnum;
   struct stat st;
   struct fio_stat fst;
@@ -812,14 +816,15 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
   statptr = (CORE_ADDR) lnum;
 
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (namelength);
-  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (namelength));
+  if (target_read_memory (nameptr, (gdb_byte *) pathname.get (),
+			  namelength) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
-  ret = stat (pathname, &st);
+  ret = stat (pathname.get (), &st);
 
   if (ret == -1)
     {
@@ -997,24 +1002,28 @@ remote_fileio_func_system (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int ret, length;
-  char *cmdline = NULL;
 
   /* Parameter: Ptr to commandline / length incl. trailing zero */
-  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
+  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length, true))
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
+  gdb::unique_xmalloc_ptr<char> cmdline_buf;
+  const char *cmdline = nullptr;
+
   if (length)
     {
       /* Request commandline using 'm' packet */
-      cmdline = (char *) alloca (length);
-      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
+      cmdline_buf.reset ((char *) xmalloc (length));
+      if (target_read_memory (ptrval, (gdb_byte *) cmdline_buf.get (),
+			      length) != 0)
 	{
 	  remote_fileio_ioerror (remote);
 	  return;
 	}
+      cmdline = cmdline_buf.get ();
     }
 
   /* Check if system(3) has been explicitly allowed using the
@@ -1244,6 +1253,70 @@ show_system_call_allowed (const char *args, int from_tty)
 	      remote_fio_system_call_allowed ? "" : "not ");
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Verify that remote_fileio_extract_ptr_w_len rejects negative and zero
+   lengths, and accepts valid ones.  The packet buffer format is
+   "ptr/len[,...]" with both fields as hex integers.  */
+
+static void
+test_remote_fileio_extract_ptr_w_len ()
+{
+  CORE_ADDR ptrval;
+  int length;
+
+  /* Build a writable "ptr/len" buffer and parse it, with and without
+     the Fsystem zero-length allowance.  */
+  auto parse = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length);
+    };
+  auto parse_allow_zero = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length, true);
+    };
+
+  /* Valid: length 1 (minimum positive).  Verify both fields are parsed.  */
+  SELF_CHECK (parse ("deadbeef/1") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 1);
+
+  /* Valid: packet with trailing fields.  */
+  SELF_CHECK (parse ("deadbeef/4,0,1a4") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 4);
+
+  /* Valid: length 0 with allow_zero_length (Fsystem).  */
+  SELF_CHECK (parse_allow_zero ("0/0") == 0);
+  SELF_CHECK (length == 0);
+
+  /* Invalid: length 0 without allow_zero_length (pathname handlers).  */
+  SELF_CHECK (parse ("0/0") == -1);
+
+  /* Valid: largest positive length accepted (technically INT_MAX).  */
+  SELF_CHECK (parse ("1/7fffffff") == 0);
+  SELF_CHECK (length == 0x7fffffff);
+
+  /* Invalid: a length whose 64-bit value is negative is rejected.  */
+  SELF_CHECK (parse ("1/8000000000000000") == -1);
+
+  /* Invalid: negative length.  */
+  SELF_CHECK (parse ("1/-1") == -1);
+
+  /* Invalid: missing '/' separator.  */
+  SELF_CHECK (parse ("deadbeef") == -1);
+}
+
+} /* namespace selftests */
+
+#endif /* GDB_SELF_TEST */
+
 void
 initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 			  struct cmd_list_element **remote_show_cmdlist)
@@ -1256,4 +1329,8 @@ initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 	   show_system_call_allowed,
 	   _("Show if the host system(3) call is allowed for the target."),
 	   remote_show_cmdlist);
+#if GDB_SELF_TEST
+  selftests::register_test ("remote_fileio_extract_ptr_w_len",
+			    selftests::test_remote_fileio_extract_ptr_w_len);
+#endif
 }
-- 
2.34.1

Re: [PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

[Machado, Luis]

Thanks. Good points. Sent a v2 now using gdb::unique_xmalloc_ptr<char> and adjusted the selftests some.

________________________________________
From: Pedro Alves <pedro@palves.net>
Sent: Wednesday, 1 July 2026 14:03
To: Machado, Luis; gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb: replace alloca with gdb::char_vector in remote-fileio.c

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


On 2026-06-30 10:52, Luis Machado wrote:

>  static int
> -remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
> +remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
> +                               bool allow_zero_length = false)
>  {
>    char *c;
>    LONGEST retlong;
> @@ -211,6 +215,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
>    *buf = c;
>    if (remote_fileio_extract_long (buf, &retlong))
>      return -1;
> +  /* Reject negative lengths, zero (unless the caller permits it for the
> +     Fsystem NULL-cmdline sentinel), and lengths above PATH_MAX to prevent
> +     out-of-bounds memory reads or oversized heap allocations.  */
> +  if (retlong < 0 || (!allow_zero_length && retlong == 0) || retlong > PATH_MAX)
> +    return -1;


PATH_MAX is NOT guaranteed to exist.  It's a portability hazard.  Note how the only place in
common code that uses it, in inf-child.c, has "#if defined (PATH_MAX)" guarding it.
The uses in remote-fileio.c itself are guarded by __CYGWIN__.

POSIX allows PATH_MAX to be undefined when the limit is indeterminate.  GNU/Hurd is the canonical case,
it has no PATH_MAX at all.

On Windows, there's MAX_PATH (not a typo, it's really the reserve word order of PATH_MAX), defined as 260,
though syscalls allow more than that.  gnulib's import/pathmax.h does give us PATH_MAX on Windows (but not
on Hurd, see comments there), but also defined as 260.

On macOS PATH_MAX is 1024, but some syscalls accept longer.

Etc.

The GNU conventions is just to not code a limit, and use dynamic allocation.  The patch already switches
from alloca to the heap, so we can just drop the PATH_MAX cap.  The buffers are transient and
passed to open, stat etc. syscalls immediately, and release immediately.  The syscalls fail with ENAMETOOLONG
if truly passed a too-long name, so the cap adds zero safety.


>    *length = (int) retlong;
>    return 0;
>  }
> @@ -305,7 +314,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>    long num;
>    int flags, fd;
>    mode_t mode;
> -  char *pathname;
>    struct stat st;
>
>    /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
> @@ -339,8 +347,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
>      }
>
>    /* Request pathname.  */
> -  pathname = (char *) alloca (length);
> -  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
> +  gdb::char_vector pathname (length);

"pathname" never needs to be resized, so std::vector adds a bit of useless weight (it needs to
track storage vs size separately, etc.).  Make it a gdb::unique_xmalloc_ptr<char> instead.
That's the only change needed, all the rest of the code will work the same.

(Ditto for all the other instances.  You do have one resize call further below, but that's
not a real resize, it's still just initialization.)

Re: [PATCH v2] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Pedro Alves]

On 2026-07-01 16:07, Luis Machado wrote:

>    char *c;
>    LONGEST retlong;
> @@ -211,6 +213,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
>    *buf = c;
>    if (remote_fileio_extract_long (buf, &retlong))
>      return -1;
> +  /* Reject negative lengths and zero (unless the caller permits it for the
> +     Fsystem NULL-cmdline sentinel).  Oversized names are caught by the
> +     syscall.  */
> +  if (retlong < 0 || (!allow_zero_length && retlong == 0))
> +    return -1;

Given this no longer limits on PATH_MAX, RETLONG can be higher than INT_MAX
and still positive, and then the cast below yields bad values:

>    *length = (int) retlong;

Like, e.g., (int) 0x80000000, wraps to INT_MIN (a negative length).

So we should limit on INT_MAX.
> +  /* Valid: largest positive length accepted (technically INT_MAX).  */
> +  SELF_CHECK (parse ("1/7fffffff") == 0);
> +  SELF_CHECK (length == 0x7fffffff);

This test would have caught it, I think:

  SELF_CHECK (parse ("1/80000000") == -1);

Re: [PATCH v2] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Machado, Luis]

At some point I think I did have the INT_MAX boundary check, but dropped it for some reason. Probably in favor of the larger 64-bit value.

V3 on its way.

Thanks,
Luis

________________________________________
From: Pedro Alves <pedro@palves.net>
Sent: Wednesday, 1 July 2026 19:20
To: Machado, Luis; gdb-patches@sourceware.org
Cc: guinevere@redhat.com
Subject: Re: [PATCH v2] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


On 2026-07-01 16:07, Luis Machado wrote:

>    char *c;
>    LONGEST retlong;
> @@ -211,6 +213,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
>    *buf = c;
>    if (remote_fileio_extract_long (buf, &retlong))
>      return -1;
> +  /* Reject negative lengths and zero (unless the caller permits it for the
> +     Fsystem NULL-cmdline sentinel).  Oversized names are caught by the
> +     syscall.  */
> +  if (retlong < 0 || (!allow_zero_length && retlong == 0))
> +    return -1;

Given this no longer limits on PATH_MAX, RETLONG can be higher than INT_MAX
and still positive, and then the cast below yields bad values:

>    *length = (int) retlong;

Like, e.g., (int) 0x80000000, wraps to INT_MIN (a negative length).

So we should limit on INT_MAX.
> +  /* Valid: largest positive length accepted (technically INT_MAX).  */
> +  SELF_CHECK (parse ("1/7fffffff") == 0);
> +  SELF_CHECK (length == 0x7fffffff);

This test would have caught it, I think:

  SELF_CHECK (parse ("1/80000000") == -1);

[PATCH v3] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Luis Machado]

remote_fileio_extract_ptr_w_len parsed a debuggee controlled 64 bit
length, narrowed it to int with no bounds check, and the value flowed
directly into alloca() across RSP File I/O handlers (Fopen, Frename,
Funlink, Fstat, Fsystem).  A malicious inferior or remote stub could
send a crafted packet with a large path length, displacing the stack
pointer by up to 2 GiB and potentially crashing the debugger.

remote_fileio_extract_ptr_w_len now rejects negative lengths, lengths
that would overflow the int result (greater than INT_MAX), and zero.
An allow_zero_length flag lets the Fsystem handler opt in to the zero
length sentinel that signals a NULL cmdline query.  All other handlers
use the default and are protected without any per call checks.  Oversized
names are rejected naturally by the underlying syscall with ENAMETOOLONG.
All alloca(length) calls have been replaced with
gdb::unique_xmalloc_ptr<char>, so an oversized but positive length now
fails as a graceful heap allocation error rather than corrupting the
stack.

Add a selftest that exercises various problematic cases.

Signed-off-by: Luis Machado <luis.machado@amd.com>
---
 gdb/remote-fileio.c | 142 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 111 insertions(+), 31 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index ef608cabfab..a151161371d 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -31,6 +31,7 @@
 #include "target.h"
 #include "filenames.h"
 #include "gdbsupport/filestuff.h"
+#include "gdbsupport/selftest.h"
 
 #include <fcntl.h>
 #include "gdbsupport/gdb_sys_time.h"
@@ -194,7 +195,8 @@ remote_fileio_extract_int (char **buf, long *retint)
 }
 
 static int
-remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
+remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length,
+				  bool allow_zero_length = false)
 {
   char *c;
   LONGEST retlong;
@@ -211,6 +213,11 @@ remote_fileio_extract_ptr_w_len (char **buf, CORE_ADDR *ptrval, int *length)
   *buf = c;
   if (remote_fileio_extract_long (buf, &retlong))
     return -1;
+  /* Reject negative lengths, values that would overflow the int length and
+     zero (unless the caller permits it for the Fsystem NULL-cmdline sentinel).
+     Oversized names are caught by the syscall.  */
+  if (retlong < 0 || retlong > INT_MAX || (!allow_zero_length && retlong == 0))
+    return -1;
   *length = (int) retlong;
   return 0;
 }
@@ -305,7 +312,6 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   long num;
   int flags, fd;
   mode_t mode;
-  char *pathname;
   struct stat st;
 
   /* 1. Parameter: Ptr to pathname / length incl. trailing zero.  */
@@ -339,8 +345,8 @@ remote_fileio_func_open (remote_target *remote, char *buf)
     }
 
   /* Request pathname.  */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (length));
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.get (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -349,7 +355,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
   /* Check if pathname exists and is not a regular file or directory.  If so,
      return an appropriate error code.  Same for trying to open directories
      for writing.  */
-  if (!stat (pathname, &st))
+  if (!stat (pathname.get (), &st))
     {
       if (!S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
 	{
@@ -364,7 +370,7 @@ remote_fileio_func_open (remote_target *remote, char *buf)
 	}
     }
 
-  fd = gdb_open_cloexec (pathname, flags, mode).release ();
+  fd = gdb_open_cloexec (pathname.get (), flags, mode).release ();
   if (fd < 0)
     {
       remote_fileio_return_errno (remote, -1);
@@ -659,7 +665,6 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 {
   CORE_ADDR old_ptr, new_ptr;
   int old_len, new_len;
-  char *oldpath, *newpath;
   int ret, of, nf;
   struct stat ost, nst;
 
@@ -678,24 +683,24 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
     }
 
   /* Request oldpath using 'm' packet */
-  oldpath = (char *) alloca (old_len);
-  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
+  gdb::unique_xmalloc_ptr<char> oldpath ((char *) xmalloc (old_len));
+  if (target_read_memory (old_ptr, (gdb_byte *) oldpath.get (), old_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Request newpath using 'm' packet */
-  newpath = (char *) alloca (new_len);
-  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
+  gdb::unique_xmalloc_ptr<char> newpath ((char *) xmalloc (new_len));
+  if (target_read_memory (new_ptr, (gdb_byte *) newpath.get (), new_len) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
   /* Only operate on regular files and directories.  */
-  of = stat (oldpath, &ost);
-  nf = stat (newpath, &nst);
+  of = stat (oldpath.get (), &ost);
+  nf = stat (newpath.get (), &nst);
   if ((!of && !S_ISREG (ost.st_mode) && !S_ISDIR (ost.st_mode))
       || (!nf && !S_ISREG (nst.st_mode) && !S_ISDIR (nst.st_mode)))
     {
@@ -703,7 +708,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
       return;
     }
 
-  ret = rename (oldpath, newpath);
+  ret = rename (oldpath.get (), newpath.get ());
 
   if (ret == -1)
     {
@@ -726,10 +731,10 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
 		  char newfullpath[PATH_MAX];
 		  int len;
 
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath, oldfullpath,
-				    PATH_MAX);
-		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath, newfullpath,
-				    PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, oldpath.get (),
+				    oldfullpath, PATH_MAX);
+		  cygwin_conv_path (CCP_WIN_A_TO_POSIX, newpath.get (),
+				    newfullpath, PATH_MAX);
 		  len = strlen (oldfullpath);
 		  if (IS_DIR_SEPARATOR (newfullpath[len])
 		      && !filename_ncmp (oldfullpath, newfullpath, len))
@@ -752,7 +757,6 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int length;
-  char *pathname;
   int ret;
   struct stat st;
 
@@ -763,8 +767,8 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
       return;
     }
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (length);
-  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (length));
+  if (target_read_memory (ptrval, (gdb_byte *) pathname.get (), length) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
@@ -772,13 +776,14 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
 
   /* Only operate on regular files (and directories, which allows to return
      the correct return code).  */
-  if (!stat (pathname, &st) && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))
+  if (!stat (pathname.get (), &st) && !S_ISREG (st.st_mode)
+      && !S_ISDIR (st.st_mode))
     {
       remote_fileio_reply (remote, -1, FILEIO_ENODEV);
       return;
     }
 
-  ret = unlink (pathname);
+  ret = unlink (pathname.get ());
 
   if (ret == -1)
     remote_fileio_return_errno (remote, -1);
@@ -791,7 +796,6 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
 {
   CORE_ADDR statptr, nameptr;
   int ret, namelength;
-  char *pathname;
   LONGEST lnum;
   struct stat st;
   struct fio_stat fst;
@@ -812,14 +816,15 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
   statptr = (CORE_ADDR) lnum;
 
   /* Request pathname using 'm' packet */
-  pathname = (char *) alloca (namelength);
-  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
+  gdb::unique_xmalloc_ptr<char> pathname ((char *) xmalloc (namelength));
+  if (target_read_memory (nameptr, (gdb_byte *) pathname.get (),
+			  namelength) != 0)
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
-  ret = stat (pathname, &st);
+  ret = stat (pathname.get (), &st);
 
   if (ret == -1)
     {
@@ -997,24 +1002,28 @@ remote_fileio_func_system (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int ret, length;
-  char *cmdline = NULL;
 
   /* Parameter: Ptr to commandline / length incl. trailing zero */
-  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
+  if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length, true))
     {
       remote_fileio_ioerror (remote);
       return;
     }
 
+  gdb::unique_xmalloc_ptr<char> cmdline_buf;
+  const char *cmdline = nullptr;
+
   if (length)
     {
       /* Request commandline using 'm' packet */
-      cmdline = (char *) alloca (length);
-      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
+      cmdline_buf.reset ((char *) xmalloc (length));
+      if (target_read_memory (ptrval, (gdb_byte *) cmdline_buf.get (),
+			      length) != 0)
 	{
 	  remote_fileio_ioerror (remote);
 	  return;
 	}
+      cmdline = cmdline_buf.get ();
     }
 
   /* Check if system(3) has been explicitly allowed using the
@@ -1244,6 +1253,73 @@ show_system_call_allowed (const char *args, int from_tty)
 	      remote_fio_system_call_allowed ? "" : "not ");
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Verify that remote_fileio_extract_ptr_w_len rejects negative and zero
+   lengths, and accepts valid ones.  The packet buffer format is
+   "ptr/len[,...]" with both fields as hex integers.  */
+
+static void
+test_remote_fileio_extract_ptr_w_len ()
+{
+  CORE_ADDR ptrval;
+  int length;
+
+  /* Build a writable "ptr/len" buffer and parse it, with and without
+     the Fsystem zero-length allowance.  */
+  auto parse = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length);
+    };
+  auto parse_allow_zero = [&] (const char *s) -> int
+    {
+      std::string buf (s);
+      char *p = buf.data ();
+      return remote_fileio_extract_ptr_w_len (&p, &ptrval, &length, true);
+    };
+
+  /* Valid: length 1 (minimum positive).  Verify both fields are parsed.  */
+  SELF_CHECK (parse ("deadbeef/1") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 1);
+
+  /* Valid: packet with trailing fields.  */
+  SELF_CHECK (parse ("deadbeef/4,0,1a4") == 0);
+  SELF_CHECK (ptrval == 0xdeadbeef);
+  SELF_CHECK (length == 4);
+
+  /* Valid: length 0 with allow_zero_length (Fsystem).  */
+  SELF_CHECK (parse_allow_zero ("0/0") == 0);
+  SELF_CHECK (length == 0);
+
+  /* Invalid: length 0 without allow_zero_length (pathname handlers).  */
+  SELF_CHECK (parse ("0/0") == -1);
+
+  /* Valid: largest positive length accepted (technically INT_MAX).  */
+  SELF_CHECK (parse ("1/7fffffff") == 0);
+  SELF_CHECK (length == 0x7fffffff);
+
+  /* Invalid: one past INT_MAX overflows the int length.  */
+  SELF_CHECK (parse ("1/80000000") == -1);
+
+  /* Invalid: a length whose 64-bit value is negative is rejected.  */
+  SELF_CHECK (parse ("1/8000000000000000") == -1);
+
+  /* Invalid: negative length.  */
+  SELF_CHECK (parse ("1/-1") == -1);
+
+  /* Invalid: missing '/' separator.  */
+  SELF_CHECK (parse ("deadbeef") == -1);
+}
+
+} /* namespace selftests */
+
+#endif /* GDB_SELF_TEST */
+
 void
 initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 			  struct cmd_list_element **remote_show_cmdlist)
@@ -1256,4 +1332,8 @@ initialize_remote_fileio (struct cmd_list_element **remote_set_cmdlist,
 	   show_system_call_allowed,
 	   _("Show if the host system(3) call is allowed for the target."),
 	   remote_show_cmdlist);
+#if GDB_SELF_TEST
+  selftests::register_test ("remote_fileio_extract_ptr_w_len",
+			    selftests::test_remote_fileio_extract_ptr_w_len);
+#endif
 }
-- 
2.34.1

Re: [PATCH v3] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Pedro Alves]

On 2026-07-01 20:01, Luis Machado wrote:
> remote_fileio_extract_ptr_w_len parsed a debuggee controlled 64 bit
> length, narrowed it to int with no bounds check, and the value flowed
> directly into alloca() across RSP File I/O handlers (Fopen, Frename,
> Funlink, Fstat, Fsystem).  A malicious inferior or remote stub could
> send a crafted packet with a large path length, displacing the stack
> pointer by up to 2 GiB and potentially crashing the debugger.
> 
> remote_fileio_extract_ptr_w_len now rejects negative lengths, lengths
> that would overflow the int result (greater than INT_MAX), and zero.
> An allow_zero_length flag lets the Fsystem handler opt in to the zero
> length sentinel that signals a NULL cmdline query.  All other handlers
> use the default and are protected without any per call checks.  Oversized
> names are rejected naturally by the underlying syscall with ENAMETOOLONG.
> All alloca(length) calls have been replaced with
> gdb::unique_xmalloc_ptr<char>, so an oversized but positive length now
> fails as a graceful heap allocation error rather than corrupting the
> stack.
> 
> Add a selftest that exercises various problematic cases.
> 
> Signed-off-by: Luis Machado <luis.machado@amd.com>

Approved-By: Pedro Alves <pedro@palves.net>

Re: [PATCH v3] gdb: replace alloca with gdb::unique_xmalloc_ptr in remote-fileio.c

[Luis]

On 01/07/2026 20:08, Pedro Alves wrote:
> On 2026-07-01 20:01, Luis Machado wrote:
>> remote_fileio_extract_ptr_w_len parsed a debuggee controlled 64 bit
>> length, narrowed it to int with no bounds check, and the value flowed
>> directly into alloca() across RSP File I/O handlers (Fopen, Frename,
>> Funlink, Fstat, Fsystem).  A malicious inferior or remote stub could
>> send a crafted packet with a large path length, displacing the stack
>> pointer by up to 2 GiB and potentially crashing the debugger.
>>
>> remote_fileio_extract_ptr_w_len now rejects negative lengths, lengths
>> that would overflow the int result (greater than INT_MAX), and zero.
>> An allow_zero_length flag lets the Fsystem handler opt in to the zero
>> length sentinel that signals a NULL cmdline query.  All other handlers
>> use the default and are protected without any per call checks.  Oversized
>> names are rejected naturally by the underlying syscall with ENAMETOOLONG.
>> All alloca(length) calls have been replaced with
>> gdb::unique_xmalloc_ptr<char>, so an oversized but positive length now
>> fails as a graceful heap allocation error rather than corrupting the
>> stack.
>>
>> Add a selftest that exercises various problematic cases.
>>
>> Signed-off-by: Luis Machado <luis.machado@amd.com>
> 
> Approved-By: Pedro Alves <pedro@palves.net>
> 

Thanks. Pushed now.