[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug #66296] rshd.c string overflow warning
From: |
Simon Josefsson |
Subject: |
Re: [bug #66296] rshd.c string overflow warning |
Date: |
Tue, 08 Oct 2024 08:33:35 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
> Hi,
>
> 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
>
> Thanks for reporting an issue you encountered with GNU Inetutils!
>
>> [...]
>> Date: Sat 05 Oct 2024 10:48:53 AM CDT By: Jeffrey Cliff <themusicgod1>
>> inetutils: 2.5
>> gcc: (GCC) 15.0.0 20240509 (experimental)
>>
>> 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'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.
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.
/Simon
>> if i'm reading this right, increasing the size of the "path" string by one
>> should should fix this:
>
> There is nothing to fix. Your patch just adds an usused byte to the "path"
> buffer.
>
>> --- inetutils-2.5/src/rshd.c 2023-12-29 11:34:46.000000000 -0600
>> +++ inetutils-2.5-works/src/rshd.c 2024-10-05 09:47:18.126254725 -0600
>> @@ -411,7 +411,7 @@
>> char logname[32 + sizeof ("LOGNAME=")] = "LOGNAME=";
>> char homedir[256 + sizeof ("HOME=")] = "HOME=";
>> char shell[64 + sizeof ("SHELL=")] = "SHELL=";
>> -char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")] = "PATH=";
>> +char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")+1] = "PATH=";
>> char rhost[128 + sizeof ("RHOST=")] = "RHOST=";
>>
>> #ifndef WITH_PAM
>
> Thanks,
> Erik
>
>
signature.asc
Description: PGP signature