[PATCH] gdbserver: fix overlap in sprintf argument and buffer

[PATCH] gdbserver: fix overlap in sprintf argument and buffer

[Simon Marchi]

While trying to build on Cygwin (gcc 10.2.0), I got:

      CXX    server.o
    /home/Baube/src/binutils-gdb/gdbserver/server.cc: In function ‘void handle_general_set(char*)’:
    /home/Baube/src/binutils-gdb/gdbserver/server.cc:832:12: error: ‘sprintf’ argument 3 overlaps destination object ‘own_buf’ [-Werror=restrict]
      832 |    sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
          |    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      833 |      mode);
          |      ~~~~~
    /home/Baube/src/binutils-gdb/gdbserver/server.cc:553:27: note: destination object referenced by ‘restrict’-qualified argument 1 was declared here
      553 | handle_general_set (char *own_buf)
          |                     ~~~~~~^~~~~~~

There is indeed a problem: mode points somewhere into own_buf.  And by
the time mode gets formatted as a %s, whatever it points to has been
overwritten.  I hacked gdbserver to coerce it into that error path, and
this is the resulting message:

    (gdb) p own_buf
    $1 = 0x629000000200 "E.Unknown thread-events mode requested: ad-events mode requested: 00;10:9020fdf7ff7f0000;thread:p49388.49388;core:e;\n"

Fix it by formatting the error string in an std::string first.

gdbserver/ChangeLog:

	* server.cc (handle_general_set): Don't use sprintf with
	argument overlapping buffer.

Change-Id: I4fdf05c0117f63739413dd67ddae7bd6ee414824
---
 gdbserver/server.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 4a211a481873..0d8ee199af9e 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -829,8 +829,10 @@ handle_general_set (char *own_buf)
       else
 	{
 	  /* We don't know what this mode is, so complain to GDB.  */
-	  sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
-		   mode);
+	  std::string err
+	    = string_printf ("E.Unknown thread-events mode requested: %s\n",
+			     mode);
+	  sprintf (own_buf, "%s", err.c_str ());
 	  return;
 	}
 
-- 
2.28.0

Re: [PATCH] gdbserver: fix overlap in sprintf argument and buffer

[Christian Biesinger]

On Wed, Oct 21, 2020 at 12:05 AM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>       CXX    server.o
>     /home/Baube/src/binutils-gdb/gdbserver/server.cc: In function ‘void handle_general_set(char*)’:
>     /home/Baube/src/binutils-gdb/gdbserver/server.cc:832:12: error: ‘sprintf’ argument 3 overlaps destination object ‘own_buf’ [-Werror=restrict]
>       832 |    sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
>           |    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       833 |      mode);
>           |      ~~~~~

https://sourceware.org/bugzilla/show_bug.cgi?id=26758 was filed on
this, so you should probably link to it in the commit message.

+         sprintf (own_buf, "%s", err.c_str ());

Why not use strcpy?

Christian

Re: [PATCH] gdbserver: fix overlap in sprintf argument and buffer

[Simon Marchi]

On 2020-10-21 5:50 a.m., Christian Biesinger wrote:
> On Wed, Oct 21, 2020 at 12:05 AM Simon Marchi via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>>       CXX    server.o
>>     /home/Baube/src/binutils-gdb/gdbserver/server.cc: In function ‘void handle_general_set(char*)’:
>>     /home/Baube/src/binutils-gdb/gdbserver/server.cc:832:12: error: ‘sprintf’ argument 3 overlaps destination object ‘own_buf’ [-Werror=restrict]
>>       832 |    sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
>>           |    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       833 |      mode);
>>           |      ~~~~~
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26758 was filed on
> this, so you should probably link to it in the commit message.
>
> +         sprintf (own_buf, "%s", err.c_str ());
>
> Why not use strcpy?

Good point, I just blindly used sprintf because that's what was used
before.  I'll switch to strcpy and push the patch.

Thanks!

Simon