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>
index | next in thread | previous in thread | raw e-mail
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.home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5c39c37d-8d0a-22e9-710b-2453e0dd4481>