Date: Sat, 27 Jan 2018 19:50:07 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au>, Dimitry Andric <dim@freebsd.org> Cc: Ed Schouten <ed@nuxi.nl>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328492 - head/contrib/opie/libopie Message-ID: <5c39c37d-8d0a-22e9-710b-2453e0dd4481@FreeBSD.org> In-Reply-To: <20180128093811.G4029@besplex.bde.org> References: <201801272216.w0RMGJwo057492@repo.freebsd.org> <CABh_MKn=3pRWyMHUAQkG17dQVBFEwFA2esFixPtgkCt7VE5oCw@mail.gmail.com> <7C471160-44B3-4EA6-8995-08A4EB4332A1@FreeBSD.org> <20180128093811.G4029@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@nuxi.nl> wrote: >>> >>> 2018-01-27 23:16 GMT+01:00 Pedro F. Giffuni <pfg@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5c39c37d-8d0a-22e9-710b-2453e0dd4481>