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

[Pedro Giffuni]

On 01/27/18 20:42, Bruce Evans wrote:

On Sat, 27 Jan 2018, Pedro Giffuni wrote:


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  wrote:



* [... context lost to corruption of spaces which makes it unreadable]




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.


That is the main breakage.  Using the size of the destination is very 
wrong,

since that size is intentionally 1 larger than the size of the source, to
leave space for appending a NUL.

I am considering reverting the change. Looking at other ways to get rid 
of the warning, please be patient.



...
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.


strlcpy() still seems to be intentionally left out of glibc.



glibc is not portable. I understand some systems that carry glibc also 
carry libbsd, or they can still use musl:


https://github.com/esmil/musl/blob/master/src/string/strlcpy.c


Pedro.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Bruce Evans]

On Sat, 27 Jan 2018, Pedro Giffuni wrote:


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  wrote:



* [... context lost to corruption of spaces which makes it unreadable]




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.


That is the main breakage.  Using the size of the destination is very wrong,
since that size is intentionally 1 larger than the size of the source, to
leave space for appending a NUL.


...
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.


strlcpy() still seems to be intentionally left out of glibc.

Bruce___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: 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  wrote:


2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :

   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.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Bruce Evans]

On Sat, 27 Jan 2018, Dimitry Andric wrote:


On 27 Jan 2018, at 23:20, Ed Schouten  wrote:


2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :

   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.


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.


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().

Bruce
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Ed Schouten]

2018-01-27 23:31 GMT+01:00 Dimitry Andric :
> On 27 Jan 2018, at 23:20, Ed Schouten  wrote:
>>
>> 2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :
>>>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".

Substituting 'len', 'src' and 'dst' gives me:

strncpy "copies at most 'sizeof(utmp.ut_host) + 1' characters from
'utmp.ut_host' into 'host'".

As 'utmp.ut_host' is not guaranteed to be null-terminated by POSIX*,
it can actually end up in the situation where it copies
'sizeof(utmp.ut_host) + 1' characters, which may leave 'host'
unterminated.

-- 
Ed Schouten 
Nuxi, 's-Hertogenbosch, the Netherlands
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Dimitry Andric]

On 27 Jan 2018, at 23:20, Ed Schouten  wrote:
> 
> 2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :
>>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,
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.

In any case, this is why strlcpy exists. :)

-Dimitry



signature.asc
Description: Message signed with OpenPGP

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

[Ed Schouten]

Hi Pedro,

2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni :
> 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?

-- 
Ed Schouten 
Nuxi, 's-Hertogenbosch, the Netherlands
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"