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.
> + 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.
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.
Jess
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
