svn commit: r367701 - head/lib/libutil

[Scott Long]

> On Nov 15, 2020, at 11:31 AM, Jessica Clarke <jrtc27 at 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 at 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.

Scott