Date: Sun, 15 Nov 2020 18:31:59 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Scott Long <scottl@FreeBSD.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r367701 - head/lib/libutil Message-ID: <D5492BB4-A282-4E35-B02F-1216769FDA51@freebsd.org> In-Reply-To: <202011150748.0AF7mqW3016900@repo.freebsd.org> References: <202011150748.0AF7mqW3016900@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Scott, I'm concerned by this diff; see my comments below. > 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, This is dangerous and generally a sign of something wrong. > + 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)) { 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. 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. Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D5492BB4-A282-4E35-B02F-1216769FDA51>