[PATCH] gdbserver/tracepoint.cc: work around -Wstringop-truncation error
Simon Marchi
simon.marchi@polymtl.ca
Fri Dec 10 20:50:19 GMT 2021
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
More information about the Gdb-patches
mailing list