> On Nov 15, 2020, at 11:31 AM, Jessica Clarke <jrt...@freebsd.org> wrote:
>
> Hi Scott,
> I'm concerned by this diff; see my comments below.
>
>> On 15 Nov 2020, at 07:48, Scott Long <sco...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
