[PATCH v2 1/2] gdbserver/tracepoint.cc: work around -Wstringop-truncation error

[PATCH v2 1/2] gdbserver/tracepoint.cc: work around -Wstringop-truncation error

[Simon Marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

When building gdb with  on AArch64 with g++ 11.1.0 (and some preceding
versions too), -O2 and -fsanitize=address, I get this error.

      CXX    tracepoint-ipa.o
    cc1plus: warning: command-line option ‘-Wmissing-prototypes’ is valid for C/ObjC but not for C++
    In file included from /usr/include/string.h:519,
                     from ../gnulib/import/string.h:41,
                     from /home/simark/src/binutils-gdb/gdbserver/../gdbsupport/common-defs.h:95,
                     from /home/simark/src/binutils-gdb/gdbserver/server.h:22,
                     from /home/simark/src/binutils-gdb/gdbserver/tracepoint.cc:19:
    In function ‘char* strncpy(char*, const char*, size_t)’,
        inlined from ‘int init_named_socket(const char*)’ at /home/simark/src/binutils-gdb/gdbserver/tracepoint.cc:6902:11,
        inlined from ‘int gdb_agent_socket_init()’ at /home/simark/src/binutils-gdb/gdbserver/tracepoint.cc:6953:26,
        inlined from ‘void* gdb_agent_helper_thread(void*)’ at /home/simark/src/binutils-gdb/gdbserver/tracepoint.cc:7204:41:
    /usr/include/bits/string_fortified.h:95:34: error: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ output may be truncated copying 107 bytes from a string of length 107 [-Werror=stringop-truncation]
       95 |   return __builtin___strncpy_chk (__dest, __src, __len,
          |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
       96 |                                   __glibc_objsize (__dest));
          |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~

Note that _FORTIFY_SOURCE changes the message a bit, but I get a similar
error if I use -D_FORTIFY_SOURCE=0.

I am pretty sure it's spurious and might be related to this GCC bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88780

From what I can see, we are copying from a static 108 bytes long buffer
(the global array agent_socket_name) to a 108 bytes long array,
sockaddr_un::sun_path.  I don't see anything wrong.

Still, it would make things easier if we didn't see this error.  Change
the code to check that the source string length is smaller than the
destination buffer (including space for the NULL byte) and use strcpy.

For anybody who would like to try to reproduce, the full command line
is:

    g++     -I. -I/home/simark/src/binutils-gdb/gdbserver -I/home/simark/src/binutils-gdb/gdbserver/../gdb/regformats -I/home/simark/src/binutils-gdb/gdbserver/.. -I/home/simark/src/binutils-gdb/gdbserver/../include -I/home/simark/src/binutils-gdb/gdbserver/../gdb -I/home/simark/src/binutils-gdb/gdbserver/../gnulib/import -I../gnulib/import -I/home/simark/src/binutils-gdb/gdbserver/.. -I..   -pthread -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-variable -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-error=maybe-uninitialized -Wno-mismatched-tags -Wsuggest-override -Wimplicit-fallthrough=3 -Wduplicated-cond -Wshadow=local -Wdeprecated-copy -Wdeprecated-copy-dtor -Wredundant-move -Wmissing-declarations -Wmissing-prototypes -Wstrict-null-sentinel -Wformat -Wformat-nonliteral -Werror -DGDBSERVER  -DCONFIG_UST_GDB_INTEGRATION -Drpl_strerror_r=strerror_r -Drpl_free=free -fPIC -DIN_PROCESS_AGENT -fvisibility=hidden -g3 -O2 -fsanitize=address   -c -o tracepoint-ipa.o -MT tracepoint-ipa.o -MMD -MP -MF ./.deps/tracepoint-ipa.Tpo /home/simark/src/binutils-gdb/gdbserver/tracepoint.cc

Change-Id: I18e86c0487feead7e7677e69398405e7057cf464
---
 gdbserver/tracepoint.cc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index c01973b5e61..5534584040b 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6899,8 +6899,13 @@ init_named_socket (const char *name)
 
   addr.sun_family = AF_UNIX;
 
-  strncpy (addr.sun_path, name, UNIX_PATH_MAX);
-  addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
+  if (strlen (name) >= ARRAY_SIZE (addr.sun_path))
+    {
+      warning ("socket name too long for sockaddr_un::sun_path field: %s", name);
+      return -1;
+    }
+
+  strcpy (addr.sun_path, name);
 
   result = access (name, F_OK);
   if (result == 0)
-- 
2.34.1

[PATCH v2 2/2] gdbserver/tracepoint.cc: use snprintf in gdb_agent_socket_init

[Simon Marchi]

If we modify tracepoint.cc to try to use a too long unix socket name,
for example by modifying SOCK_DIR to be:

    #define SOCK_DIR "/tmp/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut"

... trying to start an application with libinproctrace.so loaded
crashes:

    $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
    /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/common-utils.cc:69: A problem internal to GDBserver in-process agent has been detected.
    xsnprintf: Assertion `ret < size' failed.

Looking at the rest of the socket initialization code, the intent seems
to be that if something goes wrong, we warn but let the program
execute.  So crashing on this failed assertions seems against the intent.

Commit 6cebaf6e1ae4 ("use xsnprintf instead of snprintf.") changed this
code to use xsnprintf instead of snprintf, introducing this assertion.
Before that, snprintf would return a value bigger that UNIX_PATH_MAX and
the "if" after would catch it and emit a warning, which is exactly what
we want.  That change was done because LynxOS didn't have snprintf.
Since LynxOS isn't supported anymore, we can simply revert to use
snprintf there.

With this patch, we get a warning (printed by the caller of
gdb_agent_socket_init), but the prorgam keeps executing:

    $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
    ipa: could not create sync socket
    ...

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

diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 5534584040b..b7263db1416 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6947,8 +6947,8 @@ gdb_agent_socket_init (void)
 {
   int result, fd;
 
-  result = xsnprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
-		      SOCK_DIR, getpid ());
+  result = snprintf (agent_socket_name, UNIX_PATH_MAX, "%s/gdb_ust%d",
+		     SOCK_DIR, getpid ());
   if (result >= UNIX_PATH_MAX)
     {
       trace_debug ("string overflow allocating socket name");
-- 
2.34.1

Re: [PATCH v2 2/2] gdbserver/tracepoint.cc: use snprintf in gdb_agent_socket_init

[Pedro Alves]

On 2021-12-10 21:04, Simon Marchi via Gdb-patches wrote:
> If we modify tracepoint.cc to try to use a too long unix socket name,
> for example by modifying SOCK_DIR to be:
> 
>     #define SOCK_DIR "/tmp/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut"
> 
> ... trying to start an application with libinproctrace.so loaded
> crashes:
> 
>     $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
>     /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/common-utils.cc:69: A problem internal to GDBserver in-process agent has been detected.
>     xsnprintf: Assertion `ret < size' failed.
> 
> Looking at the rest of the socket initialization code, the intent seems
> to be that if something goes wrong, we warn but let the program
> execute.  So crashing on this failed assertions seems against the intent.

Yes, that's the intent.

> 
> Commit 6cebaf6e1ae4 ("use xsnprintf instead of snprintf.") changed this
> code to use xsnprintf instead of snprintf, introducing this assertion.
> Before that, snprintf would return a value bigger that UNIX_PATH_MAX and
> the "if" after would catch it and emit a warning, which is exactly what
> we want.  That change was done because LynxOS didn't have snprintf.
> Since LynxOS isn't supported anymore, we can simply revert to use
> snprintf there.
> 
> With this patch, we get a warning (printed by the caller of
> gdb_agent_socket_init), but the prorgam keeps executing:

prorgam -> program

> 
>     $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
>     ipa: could not create sync socket
>     ...
> 

Both patches LGTM.

Re: [PATCH v2 2/2] gdbserver/tracepoint.cc: use snprintf in gdb_agent_socket_init

[Simon Marchi]

On 2021-12-14 1:45 p.m., Pedro Alves wrote:
> On 2021-12-10 21:04, Simon Marchi via Gdb-patches wrote:
>> If we modify tracepoint.cc to try to use a too long unix socket name,
>> for example by modifying SOCK_DIR to be:
>>
>>     #define SOCK_DIR "/tmp/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut/salut"
>>
>> ... trying to start an application with libinproctrace.so loaded
>> crashes:
>>
>>     $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
>>     /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/common-utils.cc:69: A problem internal to GDBserver in-process agent has been detected.
>>     xsnprintf: Assertion `ret < size' failed.
>>
>> Looking at the rest of the socket initialization code, the intent seems
>> to be that if something goes wrong, we warn but let the program
>> execute.  So crashing on this failed assertions seems against the intent.
> 
> Yes, that's the intent.
> 
>>
>> Commit 6cebaf6e1ae4 ("use xsnprintf instead of snprintf.") changed this
>> code to use xsnprintf instead of snprintf, introducing this assertion.
>> Before that, snprintf would return a value bigger that UNIX_PATH_MAX and
>> the "if" after would catch it and emit a warning, which is exactly what
>> we want.  That change was done because LynxOS didn't have snprintf.
>> Since LynxOS isn't supported anymore, we can simply revert to use
>> snprintf there.
>>
>> With this patch, we get a warning (printed by the caller of
>> gdb_agent_socket_init), but the prorgam keeps executing:
> 
> prorgam -> program

Fixed.

> 
>>
>>     $ LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.6:./libinproctrace.so /bin/ls
>>     ipa: could not create sync socket
>>     ...
>>
> 
> Both patches LGTM.
> 

Pushed, thanks!

Simon