* [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
@ 2021-11-30 19:26 Simon Marchi
2021-12-04 11:17 ` Joel Brobecker
2021-12-09 18:00 ` Tom Tromey
0 siblings, 2 replies; 6+ messages in thread
From: Simon Marchi @ 2021-11-30 19:26 UTC (permalink / raw)
To: gdb-patches
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 assert that the source string length is smaller than the
destination buffer (including space for the NULL byte) and using 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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index c01973b5e61f..6a52c19d8a85 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6899,8 +6899,8 @@ 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';
+ gdb_assert (strlen (name) < UNIX_PATH_MAX);
+ strcpy (addr.sun_path, name);
result = access (name, F_OK);
if (result == 0)
--
2.33.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
2021-11-30 19:26 [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error Simon Marchi
@ 2021-12-04 11:17 ` Joel Brobecker
2021-12-09 18:00 ` Tom Tromey
1 sibling, 0 replies; 6+ messages in thread
From: Joel Brobecker @ 2021-12-04 11:17 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Joel Brobecker
Hi Simon,
On Tue, Nov 30, 2021 at 02:26:12PM -0500, Simon Marchi via Gdb-patches wrote:
> 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 assert that the source string length is smaller than the
> destination buffer (including space for the NULL byte) and using 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
I can't find a problem either.
I was thinking perhaps might be larger than 108, making our own
agent_socket_name array larger than 108, thus introducing a possible
overflow. But not really. At least not in my case.
That being said, when look at "man unix", it (strangely) documents
type struct sockaddr_un as:
struct sockaddr_un {
sa_family_t sun_family; /* AF_UNIX */
char sun_path[108]; /* Pathname */
};
The manual hardcodes the length of the sun_path member, instead
of making it dependent on UNIX_PATH_MAX.
When I look at the actual include, it is using UNIX_PATH_MAX, but
I decided to look at what the opengroup.org website says about it:
https://pubs.opengroup.org/onlinepubs/007904875/basedefs/sys/un.h.html
| char sun_path[] Socket pathname.
|
| The size of sun_path has intentionally been left undefined. This
| is because different implementations use different sizes. For
| example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a size of
| 104. Since most implementations originate from BSD versions, the
| size is typically in the range 92 to 108.
This perhaps suggests a slightly different way to make your change?
> gdbserver/tracepoint.cc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index c01973b5e61f..6a52c19d8a85 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -6899,8 +6899,8 @@ 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';
> + gdb_assert (strlen (name) < UNIX_PATH_MAX);
Could we use ARRAY_SIZE(addr.sun_path) instead of UNIX_PATH_MAX,
to avoid making the assumption the the size of the array is the same
as UNIX_PATH_MAX?
> + strcpy (addr.sun_path, name);
Other than that, the change looks good to me.
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
2021-11-30 19:26 [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error Simon Marchi
2021-12-04 11:17 ` Joel Brobecker
@ 2021-12-09 18:00 ` Tom Tromey
2021-12-09 19:35 ` Simon Marchi
1 sibling, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2021-12-09 18:00 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> When building gdb with on AArch64 with g++ 11.1.0 (and some preceding
Simon> versions too), -O2 and -fsanitize=address, I get this error.
Simon> From what I can see, we are copying from a static 108 bytes long buffer
Simon> (the global array agent_socket_name) to a 108 bytes long array,
Simon> sockaddr_un::sun_path. I don't see anything wrong.
Simon> - strncpy (addr.sun_path, name, UNIX_PATH_MAX);
Simon> - addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
Simon> + gdb_assert (strlen (name) < UNIX_PATH_MAX);
Simon> + strcpy (addr.sun_path, name);
It seems weird to add an assert, but OTOH I suppose if the length is too
long, then the previous code would also behave weirdly.
I wonder if an exception is a better choice though?
Also perhaps using the size of sun_path would be a different reasonable
approach?
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
2021-12-09 18:00 ` Tom Tromey
@ 2021-12-09 19:35 ` Simon Marchi
2021-12-10 19:18 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2021-12-09 19:35 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2021-12-09 1:00 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> When building gdb with on AArch64 with g++ 11.1.0 (and some preceding
> Simon> versions too), -O2 and -fsanitize=address, I get this error.
>
> Simon> From what I can see, we are copying from a static 108 bytes long buffer
> Simon> (the global array agent_socket_name) to a 108 bytes long array,
> Simon> sockaddr_un::sun_path. I don't see anything wrong.
>
> Simon> - strncpy (addr.sun_path, name, UNIX_PATH_MAX);
> Simon> - addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
> Simon> + gdb_assert (strlen (name) < UNIX_PATH_MAX);
> Simon> + strcpy (addr.sun_path, name);
>
> It seems weird to add an assert, but OTOH I suppose if the length is too
> long, then the previous code would also behave weirdly.
>
> I wonder if an exception is a better choice though?
> Also perhaps using the size of sun_path would be a different reasonable
> approach?
Joel also suggested to use ARRAY_SIZE to get the length of sun_path,
I'll do that.
Error reporting in this function seems to be done using warning, which
makes sense, we probably don't want to tear down the whole process if
something goes wrong (this is in libinproctrace.so). So the check would
look like:
if (strlen (name) >= ARRAY_SIZE (addr.sun_path))
{
warning ("socket name too long for sockaddr_un::sun_path field: %s", name);
return -1;
}
In practice, if the name is too long, we hit this assertion during the
xsnprintf earlier:
/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.
So in practice we won't reach the warning. I hacked things up to go
past the xsnprintf error with a long name, and confirmed that the
warning worked correctly.
See the updated patch below.
Simon
From 33314a290c0fb6032122848d77ea5d11cd90c3be Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Tue, 30 Nov 2021 14:26:12 -0500
Subject: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation
error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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 | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index c01973b5e61f..92dbbdd502c0 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6899,8 +6899,10 @@ 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);
+
+ strcpy (addr.sun_path, name);
result = access (name, F_OK);
if (result == 0)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
2021-12-09 19:35 ` Simon Marchi
@ 2021-12-10 19:18 ` Pedro Alves
2021-12-10 20:50 ` Simon Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2021-12-10 19:18 UTC (permalink / raw)
To: Simon Marchi, Tom Tromey, Simon Marchi via Gdb-patches
On 2021-12-09 19:35, Simon Marchi via Gdb-patches wrote:
> On 2021-12-09 1:00 p.m., Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Simon> When building gdb with on AArch64 with g++ 11.1.0 (and some preceding
>> Simon> versions too), -O2 and -fsanitize=address, I get this error.
>>
>> Simon> From what I can see, we are copying from a static 108 bytes long buffer
>> Simon> (the global array agent_socket_name) to a 108 bytes long array,
>> Simon> sockaddr_un::sun_path. I don't see anything wrong.
>>
>> Simon> - strncpy (addr.sun_path, name, UNIX_PATH_MAX);
>> Simon> - addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
>> Simon> + gdb_assert (strlen (name) < UNIX_PATH_MAX);
>> Simon> + strcpy (addr.sun_path, name);
>>
>> It seems weird to add an assert, but OTOH I suppose if the length is too
>> long, then the previous code would also behave weirdly.
>>
>> I wonder if an exception is a better choice though?
>> Also perhaps using the size of sun_path would be a different reasonable
>> approach?
>
> Joel also suggested to use ARRAY_SIZE to get the length of sun_path,
> I'll do that.
>
> Error reporting in this function seems to be done using warning, which
> makes sense, we probably don't want to tear down the whole process if
> something goes wrong (this is in libinproctrace.so). So the check would
> look like:
>
> if (strlen (name) >= ARRAY_SIZE (addr.sun_path))
> {
> warning ("socket name too long for sockaddr_un::sun_path field: %s", name);
> return -1;
> }
>
> In practice, if the name is too long, we hit this assertion during the
> xsnprintf earlier:
>
> /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.
>
Well, that code is trying to validate the size already:
result = xsnprintf (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");
return -1;
}
But that "result >= UNIX_PATH_MAX" code is dead code today. The bug here is in
using xsnprintf instead of raw snprintf here.
snprintf was actually how the original code in 0fb4aa4bfcc2aa61c27132f94cf1656dca137dc9 looked
like:
+ res = snprintf (path, UNIX_PATH_MAX, "%s/gdb_ust%d", SOCK_DIR, pid);
+ if (res >= UNIX_PATH_MAX)
+ {
+ trace_debug ("string overflow allocating socket name");
+ return -1;
+ }
It was later changed to xsnprintf with this:
commit 6cebaf6e1ae4a9f02d9d9136fccbab1ef06b1b6e
Author: gdbadmin <gdbadmin@sourceware.org>
AuthorDate: Wed Sep 1 01:53:43 2010 +0000
use xsnprintf instead of snprintf.
snprintf is not available on LynxOS, so I changed the calls to snprintf
to calls to xsnprintf, which should be strictly equivalent.
gdb/gdbserver/ChangeLog:
Lynx support has since been removed from gdbserver, so we can just revert
that bit here.
> So in practice we won't reach the warning. I hacked things up to go
> past the xsnprintf error with a long name, and confirmed that the
> warning worked correctly.
>
> See the updated patch below.
>
> Simon
>
>
> From 33314a290c0fb6032122848d77ea5d11cd90c3be Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Tue, 30 Nov 2021 14:26:12 -0500
> Subject: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation
> error
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> 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 | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index c01973b5e61f..92dbbdd502c0 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -6899,8 +6899,10 @@ 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);
> +
> + strcpy (addr.sun_path, name);
>
> result = access (name, F_OK);
> if (result == 0)
>
This still does the strcpy after warning, so if it ever was possible to reach it,
it'd still overflow. If we're adding a length check, then I think it should be
done at the caller, like it used to. Any check here I think should then be an
assert.
I think the original code here can just be tweaked to strncpy "UNIX_PATH_MAX - 1"
instead of UNIX_PATH_MAX, and IIUC, that would get rid of the compiler warning.
Take a look at gdb/ser-uds.c, there we only copy up to "UNIX_PATH_MAX - 1",
not UNIX_PATH_MAX:
~~~~~~~~~
if (strlen (name) > UNIX_PATH_MAX - 1)
{
warning
(_("The socket name is too long. It may be no longer than %s bytes."),
pulongest (UNIX_PATH_MAX - 1L));
return -1;
}
memset (&addr, 0, sizeof addr);
addr.sun_family = AF_UNIX;
strncpy (addr.sun_path, name, UNIX_PATH_MAX - 1);
int sock = socket (AF_UNIX, SOCK_STREAM, 0);
~~~~~~~~~
This is in accordance with https://man7.org/linux/man-pages/man7/unix.7.html man page,
which says:
~~~~~~~~~
Pathname sockets
When binding a socket to a pathname, a few rules should be
observed for maximum portability and ease of coding:
* The pathname in sun_path should be null-terminated.
* The length of the pathname, including the terminating null
byte, should not exceed the size of sun_path.
~~~~~~~~~
This comment explains that in reality, sun_path shouldn't be treated as
a null terminated string though:
https://news.ycombinator.com/item?id=17249912
... which explains why most code out there that writes to sun_path uses strncpy,
making sure the whole buffer is cleared after the string.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
2021-12-10 19:18 ` Pedro Alves
@ 2021-12-10 20:50 ` Simon Marchi
0 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2021-12-10 20:50 UTC (permalink / raw)
To: Pedro Alves, Tom Tromey, Simon Marchi via Gdb-patches
On 2021-12-10 2:18 p.m., Pedro Alves wrote:
> Lynx support has since been removed from gdbserver, so we can just revert
> that bit here.
Ok. I would do that as a separate patch, since the concern here is
really to get rid of the build warning.
>> 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 | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
>> index c01973b5e61f..92dbbdd502c0 100644
>> --- a/gdbserver/tracepoint.cc
>> +++ b/gdbserver/tracepoint.cc
>> @@ -6899,8 +6899,10 @@ 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);
>> +
>> + strcpy (addr.sun_path, name);
>>
>> result = access (name, F_OK);
>> if (result == 0)
>>
>
> This still does the strcpy after warning, so if it ever was possible to reach it,
> it'd still overflow.
Woops, my mistake, it's missing a "return -1". I did add it after
testing and realizing that, but forgot to update this.
> If we're adding a length check, then I think it should be
> done at the caller, like it used to. Any check here I think should then be an
> assert.
Well, that was my first idea too, there is already a length check in the
caller. But then based on the suggestions from Tom and Joel, I thought
it made sense to have the check close to where the sockaddr_un structure
is filled, using ARRAY_SIZE to get the actual length of the structure
field. The caller does its own check, but it's a check related to the
length of the agent_socket_name array (although I understand the fact
that agent_socket_name is the same size as sockaddr_un is not a
coincidence, but nothing enforces that).
Anyway, I don't want to spend too much time polishing a code path that
will never actually get used :).
> I think the original code here can just be tweaked to strncpy "UNIX_PATH_MAX - 1"
> instead of UNIX_PATH_MAX, and IIUC, that would get rid of the compiler warning.
This is what I tried first, but it still fails to compile:
jenkins@ci-node-deb11-arm64-01:~/smarchi/binutils-gdb/gdbserver$ git diff
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index c01973b5e6..c770225092 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6899,7 +6899,7 @@ init_named_socket (const char *name)
addr.sun_family = AF_UNIX;
- strncpy (addr.sun_path, name, UNIX_PATH_MAX);
+ strncpy (addr.sun_path, name, UNIX_PATH_MAX - 1);
addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
result = access (name, F_OK);
jenkins@ci-node-deb11-arm64-01:~/smarchi/binutils-gdb/gdbserver$ make V=1 tracepoint-ipa.o
g++ -I. -I. -I./../gdb/regformats -I./.. -I./../include -I./../gdb -I./../gnulib/import -I../gnulib/import -I./.. -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 -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 -g -O2 -fsanitize=address -c -o tracepoint-ipa.o -MT tracepoint-ipa.o -MMD -MP -MF ./.deps/tracepoint-ipa.Tpo tracepoint.cc
In file included from /usr/include/string.h:495,
from ./../gnulib/import/string.h:41,
from ./../gdbsupport/common-defs.h:95,
from server.h:22,
from tracepoint.cc:19:
In function ‘char* strncpy(char*, const char*, size_t)’,
inlined from ‘int init_named_socket(const char*)’ at tracepoint.cc:6902:11,
inlined from ‘int gdb_agent_socket_init()’ at tracepoint.cc:6953:26,
inlined from ‘void* gdb_agent_helper_thread(void*)’ at tracepoint.cc:7204:41:
/usr/include/aarch64-linux-gnu/bits/string_fortified.h:106: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]
106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make: *** [Makefile:509: tracepoint-ipa.o] Error 1
>
> Take a look at gdb/ser-uds.c, there we only copy up to "UNIX_PATH_MAX - 1",
> not UNIX_PATH_MAX:
>
> ~~~~~~~~~
> if (strlen (name) > UNIX_PATH_MAX - 1)
> {
> warning
> (_("The socket name is too long. It may be no longer than %s bytes."),
> pulongest (UNIX_PATH_MAX - 1L));
> return -1;
> }
>
> memset (&addr, 0, sizeof addr);
> addr.sun_family = AF_UNIX;
> strncpy (addr.sun_path, name, UNIX_PATH_MAX - 1);
>
> int sock = socket (AF_UNIX, SOCK_STREAM, 0);
> ~~~~~~~~~
I don't see a build failure here. I think the build failure in
tracepoint.cc is because it's all in the same file and with -O2, things
get inlined, so the compiler is able to connect the dots between the
static array of a known length and the strncpy calls. And it's not the
case here in ser-uds.c.
> This is in accordance with https://man7.org/linux/man-pages/man7/unix.7.html man page,
> which says:
>
> ~~~~~~~~~
> Pathname sockets
> When binding a socket to a pathname, a few rules should be
> observed for maximum portability and ease of coding:
>
> * The pathname in sun_path should be null-terminated.
>
> * The length of the pathname, including the terminating null
> byte, should not exceed the size of sun_path.
> ~~~~~~~~~
>
> This comment explains that in reality, sun_path shouldn't be treated as
> a null terminated string though:
>
> https://news.ycombinator.com/item?id=17249912
>
> ... which explains why most code out there that writes to sun_path uses strncpy,
> making sure the whole buffer is cleared after the string.
Ack.
I will send an updated series.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-10 20:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-30 19:26 [PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error Simon Marchi
2021-12-04 11:17 ` Joel Brobecker
2021-12-09 18:00 ` Tom Tromey
2021-12-09 19:35 ` Simon Marchi
2021-12-10 19:18 ` Pedro Alves
2021-12-10 20:50 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).