bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH unionfs 3/3] Don't use strncat() with length derived from sou


From: Sergey Bugaev
Subject: Re: [PATCH unionfs 3/3] Don't use strncat() with length derived from source string
Date: Tue, 27 Apr 2021 11:57:03 +0300

On Mon, Apr 26, 2021 at 11:10 PM Samuel Thibault
<samuel.thibault@gnu.org> wrote:
> Err, but wouldn't the compiler be able to determine that the size was
> properly computed, and avoid emitting a false-positive warning?

It is my understanding, based on
https://gcc.gnu.org/bugzilla//show_bug.cgi?id=88059, that GCC does not
do any sophisticated analysis here, and just warns about any case
where the specified length depends on the source size. Which makes
sense to me, because either the destination buffer size depends on the
source string length, in which case you can be sure it fits and don't
need strncpy, or it does not depend on the source string length, in
which case the string might not fit and you'd use strncpy, passing the
destination buffer size.

> The compiler emitting a warning looks to me like a sign that there might
> really be something subtly wrong. Actually sending the output of the
> compiler on the list would help us to know more about it.

Here are the warnings I get (gcc --version is gcc (Debian
10.2.1-6+hurd.1) 10.2.1 20210110):

lib.c: In function ‘make_filepath’:
lib.c:154:3: warning: ‘strncpy’ specified bound depends on the length
of the source argument [-Wstringop-overflow=]
  154 |   strncpy (filepath, path, length);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib.c:149:12: note: length computed here
  149 |   length = strlen (path) + strlen (filename) + 2;
      |            ^~~~~~~~~~~~~
lib.c:155:3: warning: ‘strncat’ specified bound depends on the length
of the source argument [-Wstringop-overflow=]
  155 |   strncat (filepath, filename, strlen (filename));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
options.c: In function ‘argp_parse_common_options’:
options.c:77:5: warning: variable ‘ulfs_match’ set but not used
[-Wunused-but-set-variable]
   77 |     ulfs_match = 0, ulfs_priority = 0;
      |     ^~~~~~~~~~
stow.c: In function ‘stow_diradd’:
stow.c:290:7: warning: ‘strncpy’ output truncated before terminating
nul copying as many bytes from a string as its length
[-Wstringop-truncation]
  290 |       strncpy (tmp, dir, dir_len);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
stow.c:275:13: note: length computed here
  275 |   dir_len = strlen(dir);
      |             ^~~~~~~~~~~

The last one is indeed a false positive, because we (with my previous
patch merged) now null-terminate the resulting string explicitly.

-- 
Sergey



reply via email to

[Prev in Thread] Current Thread [Next in Thread]