Date: Sun, 15 Nov 2020 19:01:01 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Scott Long <scottl@samsco.org> Cc: Scott Long <scottl@FreeBSD.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r367701 - head/lib/libutil Message-ID: <329C4753-BB97-4C67-8CDA-39EB67E16CE8@freebsd.org> In-Reply-To: <A39C12CC-D3D6-4166-9089-7466FA1C2B2D@samsco.org> References: <202011150748.0AF7mqW3016900@repo.freebsd.org> <D5492BB4-A282-4E35-B02F-1216769FDA51@freebsd.org> <A39C12CC-D3D6-4166-9089-7466FA1C2B2D@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 15 Nov 2020, at 18:46, Scott Long <scottl@samsco.org> wrote: >> On Nov 15, 2020, at 11:31 AM, Jessica Clarke <jrtc27@FreeBSD.org> = wrote: >>=20 >> Hi Scott, >> I'm concerned by this diff; see my comments below. >>=20 >>> On 15 Nov 2020, at 07:48, Scott Long <scottl@FreeBSD.org> wrote: >>>=20 >>> Author: scottl >>> Date: Sun Nov 15 07:48:52 2020 >>> New Revision: 367701 >>> URL: https://svnweb.freebsd.org/changeset/base/367701 >>>=20 >>> Log: >>> Because getlocalbase() returns -1 on error, it needs to use a signed = type >>> internally. Do that, and make sure that conversations between = signed and >>> unsigned don't overflow >>>=20 >>> Modified: >>> head/lib/libutil/getlocalbase.c >>>=20 >>> Modified: head/lib/libutil/getlocalbase.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- head/lib/libutil/getlocalbase.c Sun Nov 15 01:54:44 2020 = (r367700) >>> +++ head/lib/libutil/getlocalbase.c Sun Nov 15 07:48:52 2020 = (r367701) >>> @@ -41,7 +41,7 @@ __FBSDID("$FreeBSD$"); >>> ssize_t >>> getlocalbase(char *path, size_t pathlen) >>> { >>> - size_t tmplen; >>> + ssize_t tmplen; >>> const char *tmppath; >>>=20 >>> if ((pathlen =3D=3D 0) || (path =3D=3D NULL)) { >>> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen) >>> return (-1); >>> } >>>=20 >>> + /* It's unlikely that the buffer would be this big */ >>> + if (pathlen > SSIZE_MAX) { >>> + errno =3D ENOMEM; >>> + return (-1); >>> + } >>> + >>> tmppath =3D NULL; >>> - tmplen =3D pathlen; >>> + tmplen =3D (size_t)pathlen; >>> if (issetugid() =3D=3D 0) >>> tmppath =3D getenv("LOCALBASE"); >>>=20 >>> if ((tmppath =3D=3D NULL) && >>> - (sysctlbyname("user.localbase", path, &tmplen, NULL, 0) =3D=3D= 0)) { >>> + (sysctlbyname("user.localbase", path, (size_t *)&tmplen, = NULL, >>=20 >> This is dangerous and generally a sign of something wrong. >=20 > I think that the danger was mitigated by the overflow check above, but = I > agree that this is generally a poor pattern. >=20 >>=20 >>> + 0) =3D=3D 0)) { >>> return (tmplen); >>> } >>>=20 >>> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen) >>> #endif >>>=20 >>> tmplen =3D strlcpy(path, tmppath, pathlen); >>> - if ((tmplen < 0) || (tmplen >=3D pathlen)) { >>> + if ((tmplen < 0) || (tmplen >=3D (ssize_t)pathlen)) { >>=20 >> As I commented on the previous commit (but which you do not appear to >> have picked up on), the LHS is impossible. Of course, now the types >> have changed so the compiler doesn't know that. >>=20 >=20 > The man page for strlcpy() made reference to the return value being > equivalent to what snprintf() does. The man page for snprintf() = states > that negatve return values are possible, so I assumed the same was > true for strlcpy(). However, now that I=E2=80=99ve looked at the = implementation > of strlcpy(), I see that you=E2=80=99re correct. The man pages are = definitely > confusing, and this isn=E2=80=99t the only place where I think = there=E2=80=99s > inconsistency in the documentation, or at least poor wording choices. >=20 >> I think tmplen should have remained a size_t, as everywhere it's used >> you're having to cast, which is generally a sign you've done = something >> wrong. Only when you come to return from the function do you need a >> single cast to ssize_t (provided you've checked for overflow first). = I >> strongly believe this entire commit should be reverted, and whatever >> bug you were trying to fixed be fixed in another way without using = the >> wrong types and adding an array of unnecessary and/or dangerous = casts. >>=20 >=20 > I felt similar concerns, but my misunderstanding of strlcpy() drove = the > result. Since the use case for getlocalbase() lends itself to also = use > strlcat()/strlcpy(), I was trying to replicate the API semantics of = those, > at least to the limit of my understanding. Thanks for the feedback, = I=E2=80=99ll > look at it some more. Thanks. ENOMEM also feels inappropriate as no allocation is taking place. Perhaps ENAMETOOLONG, which is used in similar cases for things like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh. Also, if pathlen has already been checked against SSIZE_MAX (giving EINVAL) and tmplen against pathlen there's no need to then check tmplen against SSIZE_MAX. I'd be happy to give a review on Phabricator if/when you have a new patch. Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?329C4753-BB97-4C67-8CDA-39EB67E16CE8>