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

[Guinevere Larsen <guinevere@redhat.com>]

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