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>
index | next in thread | previous in thread | raw e-mail
> On Nov 15, 2020, at 11:31 AM, Jessica Clarke <jrtc27@FreeBSD.org> wrote: > > 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: >> >> Author: scottl >> Date: Sun Nov 15 07:48:52 2020 >> New Revision: 367701 >> URL: https://svnweb.freebsd.org/changeset/base/367701 >> >> 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 >> >> Modified: >> head/lib/libutil/getlocalbase.c >> >> Modified: head/lib/libutil/getlocalbase.c >> ============================================================================== >> --- 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; >> >> if ((pathlen == 0) || (path == NULL)) { >> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen) >> return (-1); >> } >> >> + /* It's unlikely that the buffer would be this big */ >> + if (pathlen > SSIZE_MAX) { >> + errno = ENOMEM; >> + return (-1); >> + } >> + >> tmppath = NULL; >> - tmplen = pathlen; >> + tmplen = (size_t)pathlen; >> if (issetugid() == 0) >> tmppath = getenv("LOCALBASE"); >> >> if ((tmppath == NULL) && >> - (sysctlbyname("user.localbase", path, &tmplen, NULL, 0) == 0)) { >> + (sysctlbyname("user.localbase", path, (size_t *)&tmplen, NULL, > > 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. > >> + 0) == 0)) { >> return (tmplen); >> } >> >> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen) >> #endif >> >> tmplen = strlcpy(path, tmppath, pathlen); >> - if ((tmplen < 0) || (tmplen >= pathlen)) { >> + if ((tmplen < 0) || (tmplen >= (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. > 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’ve looked at the implementation of strlcpy(), I see that you’re correct. The man pages are definitely confusing, and this isn’t the only place where I think there’s 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. > 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’ll look at it some more. Scotthome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A39C12CC-D3D6-4166-9089-7466FA1C2B2D>