svn commit: r367701 - head/lib/libutil

[Jessica Clarke]

On 15 Nov 2020, at 18:46, Scott Long <scottl at samsco.org> wrote:
>> 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.

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