Date: Sun, 15 Nov 2020 11:46:35 -0700 From: Scott Long <scottl@samsco.org> To: Jessica Clarke <jrtc27@FreeBSD.org> Cc: Scott Long <scottl@FreeBSD.org>, 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: <A39C12CC-D3D6-4166-9089-7466FA1C2B2D@samsco.org> In-Reply-To: <D5492BB4-A282-4E35-B02F-1216769FDA51@freebsd.org> References: <202011150748.0AF7mqW3016900@repo.freebsd.org> <D5492BB4-A282-4E35-B02F-1216769FDA51@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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. I think that the danger was mitigated by the overflow check above, but I agree that this is generally a poor pattern. >=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 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. > 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 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. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A39C12CC-D3D6-4166-9089-7466FA1C2B2D>