svn commit: r328492 - head/contrib/opie/libopie

[Pedro Giffuni]

On 01/27/18 18:21, Bruce Evans wrote:
> On Sat, 27 Jan 2018, Dimitry Andric wrote:
>
>> On 27 Jan 2018, at 23:20, Ed Schouten <ed at nuxi.nl> wrote:
>>>
>>> 2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni <pfg at freebsd.org>:
>>>>        char host[sizeof(utmp.ut_host) + 1];
>>>>        insecure = 1;
>>>>
>>>> -       strncpy(host, utmp.ut_host, sizeof(utmp.ut_host));
>>>> -       host[sizeof(utmp.ut_host)] = 0;
>>>> +       strncpy(host, utmp.ut_host, sizeof(host));
>>>
>>> Wait... This may access utmp.ut_host one byte past the end and no
>>> longer guarantees that host is null-terminated, right?
>
>> No, strncpy "copies at most len characters from src into dst".  However,
>
> No, the change breaks the length so 1 byte past the end is accessed
> in implementations where ut_host is not guaranteed to be NUL terminated
> and the current instance of ut_host is not NUL terminated.
>
The main change is in the sizeof(). Regularly you should use the size of 
destination not the source, and apparently GCC8 decided there was 
something to check there.

>> if the length of the source is equal to or greater than len, the
>> destination is *not* null terminated.  This is likely why the
>> "host[sizeof(utmp.ut_host)] = 0;" statement was added.
>
> This is why that statement was there.
>
> This change is not even wrong under FreeBSD, since ut_host and several 
> other
> fields are guaranteed to be NUL terminated in the FreeBSD implementation.
> The code was correct and portable and the change just breaks its 
> portability.
>

The change was done for portability to GCC, or at least to fix a warning 
there.

>> In any case, this is why strlcpy exists. :)
>
> Using strlcpy() in libopie would be another good unportabilization.
> contrib/opie never uses strlc*() except in 1 place previously
> unportabilized in r208586.  That at least fixed 2 bugs (2 related off
> by 1 errors in the code intended to avoid buffer overruns, with the
> result that buffer overruns were limited to 1 byte).  It moved the
> style bugs by changing hacking on the source string to use of strlcpy().
>

Looking in detail, upstream (which appears to have disappeared) does 
have the explicit NULL termination in our last import. For consistency 
and given that we already have a strlcpy in that code, we should use 
strlcpy() there. Every modern OS out there has strlcpy(3) and if not 
they can figure out what to do.

Pedro.