Re: [bug #66296] rshd.c string overflow warning

[Erik Auerswald]

Hi Simon,

On Tue, Oct 08, 2024 at 08:33:35AM +0200, Simon Josefsson wrote:
> Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
> > On Sat, Oct 05, 2024 at 11:48:56AM -0400, Jeffrey Cliff wrote:
> >> URL:
> >>   <https://savannah.gnu.org/bugs/?66296>
> >> 
> >>                  Summary: rshd.c string overflow warning
> > [...]
> >> [...]
> >> Date: Sat 05 Oct 2024 10:48:53 AM CDT By: Jeffrey Cliff <themusicgod1>
> >> inetutils: 2.5
> >> gcc: (GCC) 15.0.0 20240509 (experimental)

This GCC version description confused me.

> >> rshd.c:1923:3: warning: 'strncat' specified bound 13 equals source length
> >> [-Wstringop-overflow=]
> >>  1923 |   strncat (path, PATH_DEFPATH, sizeof (path) - sizeof ("PATH=") - 
> >> 1);

I also looked at my build and did not find this warning.  Thus I thought
this is a new warning coming to stable GCC in the future, but it is
not new.

> > I'd say this is a wrong warning, because there is nothing wrong to warn
> > about.  With "src" as long as "n", strncat appends all n bytes to "dest"
> > and adds a NUL byte.  This requires the "dest" buffer to be one byte longer
> > than strlen(dest) + n.  This is the case here, as can be seen from your
> > patch.
> 
> While I agree that the warning seems weird -- but isn't this fix the
> right thing anyway?
> 
> -char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")] = "PATH=";
> +char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")+1] = "PATH=";
> 
> If PATH_DEFPATH is say "/bin" then sizeof of it is 4, and
> sizeof("PATH=") is 5, and we want the resulting concatenated string to
> be "PATH=/bin" (with terminating NUL character) which has length 5+4 and
> needs sizeof 5+4+1 for storage.

Thanks, this has motivated me to look into this a bit more closely. :-)

Since the buffer looks one byte longer than strncat() may copy from
the source string, there should not be any overflow despite strncat()
potentially adding a NUL after the last copied byte.  If the buffer size
computation is wrong, the result may be a truncated 'path' that would
not work as expected.

If 'sizeof "a string"' excludes the trailing NUL byte, the buffer
is too small.  If it includes it, the buffer is one byte too large.
Since I was not sure about this, i.e., the exact results of 'sizeof
"a string"', I wrote a small test program to check it:

----------------------------------8<-----------------------------------
#include <stdio.h> 
#include <string.h>

void main(void)
{
  char path[sizeof ("/bin") + sizeof ("PATH=")] = "PATH=";

  printf("sizeof (\"/bin\") = %ld\n", sizeof ("/bin"));
  printf("sizeof (\"PATH=\") = %ld\n", sizeof ("PATH="));
  printf("sizeof (path) = %ld\n", sizeof (path));
  printf("sizeof (\"PATH=/bin\") = %ld\n", sizeof ("PATH=/bin"));

  strncat (path, "/bin", sizeof (path) - sizeof ("PATH=") - 1);
  puts(path);
}
---------------------------------->8-----------------------------------

When compiling it, the mentioned warning is emitted even with an
older GCC:

    $ gcc --version | head -n1
    gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
    $ gcc -o s s.c
    s.c: In function ‘main’:
    s.c:13:3: warning: ‘strncat’ specified bound 4 equals source length 
[-Wstringop-overflow=]
       13 |   strncat (path, "/bin", sizeof (path) - sizeof ("PATH=") - 1);
          |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The test program's output is as follows:

    $ ./s
    sizeof ("/bin") = 5
    sizeof ("PATH=") = 6
    sizeof (path) = 11
    sizeof ("PATH=/bin") = 10
    PATH=/bin

This shows:

  * 'sizeof "a string"' includes the trailing NUL byte
  * the path buffer was originally sized to hold two NUL bytes
  * the strncat() length parameter excluded the source's NUL byte
  * there was no overflow
  * there was no truncation

The patch that silences the warning increased the buffer by another byte
(now there are two additional bytes).  Additionally, the NUL from the
source is copied by strncat() instead of adding it after copying all
but the NUL byte.

> Maybe I'm confused by the strange strncat call.  I'm happy to revert
> this, and I'm sorry I didn't reproduce and test things more carefully
> before installing the change.

I'd say we should keep the change.  The commit log only states facts: a
GCC 15 warning is fixed and a buffer size is increased.  The buffer size
was not exact before, now it is just a little bit less exact than before.

It may be possible to replace strncat() by using the C preprocessor's
string concatenation, since at least at first glance it is only used to
concatenate two static strings.  I did not investigate this possibility.
I do not use rshd and I do not like to add optimizations to code I cannot
easily test.

I am still wondering why this warning is emitted for rshd.c by GCC 15,
but not by the GCC from Ubuntu LTS 20.04.

IMHO this GCC warning is problematic, because using an exact size buffer
as strncat() destination requires omitting the NUL from the source string
in order to ensure that strncat() does not overflow the destination
buffer even if the source string is somehow changed without adjusting the
destination buffer.  Thus correct strncat() use triggers this warning.
But the warning is quite old already and not likely to go away again.
Working around it thus seems acceptable to me.

Using any of the string functions with an "n" in their name is tricky,
since they all seem to have slightly different rules.  The warnings
emitted by GCC make this situation even worse.  I still prefer to use
bounded string functions over their unbounded counterparts.

Br,
Erik
-- 
The computing scientist’s main challenge is not to get confused by
the complexities of his own making.
                        -- Edsger W. Dijkstra