From owner-svn-src-head@freebsd.org Sun Nov 15 19:01:04 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id D0A61465101 for ; Sun, 15 Nov 2020 19:01:04 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CZ1km5R80z3H94 for ; Sun, 15 Nov 2020 19:01:04 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wm1-f52.google.com with SMTP id h2so21888632wmm.0 for ; Sun, 15 Nov 2020 11:01:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=l/scEbOq+GqM12vTmPSGPdm/L9AXpYo3gXsCXew+KvI=; b=HQ6JZaGuuXub+hTMe1hiGsLN465A/cUxQkddBhP5gNDGX9dqOUaGng6PEPtg6GHK9Q eDmG+8QUPuiLKG3L9Cfo2swCk9HgmP9vVeM23bwWKthbwMTIj2MeI5zWxZl4pKOB+rDj fF4alS/fRqgFDZKhNvgOK+a898oxfHKechXYJIYtW4dxwUVn9dPiCM7hwF9/niJSIn0m eoD/Q/ElxH2iNw4ZB71igqSEfls3qGKNHqauUdkqxzPMtZu+DydGRoZ5J82IaNKs+pV7 pRitfEyrpIr08qo49dnetmPid2TnJOoLwV30J56xZ+G+ieQuRvCkwzdJQn++FF8ii/0a gZKA== X-Gm-Message-State: AOAM531GaD6yPMIn0NlWBFPQJZ2Izk1P0JSOFleviJTvjovJJpef1YVH 4HkUoxATrQoAT7sUsgYdCL+0HQ== X-Google-Smtp-Source: ABdhPJxJyGOZgRnGm/p/qBVVZrLgPtqftI+VPxDW3zEFt05VKmxvKaljz+faAij+KZ7ZmvGgPqkHMA== X-Received: by 2002:a1c:a185:: with SMTP id k127mr11727518wme.23.1605466863257; Sun, 15 Nov 2020 11:01:03 -0800 (PST) Received: from [192.168.149.251] (trinity-students-nat.trin.cam.ac.uk. [131.111.193.104]) by smtp.gmail.com with ESMTPSA id n9sm16968667wmd.4.2020.11.15.11.01.02 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 15 Nov 2020 11:01:02 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: svn commit: r367701 - head/lib/libutil From: Jessica Clarke In-Reply-To: Date: Sun, 15 Nov 2020 19:01:01 +0000 Cc: Scott Long , src-committers , svn-src-all , svn-src-head Content-Transfer-Encoding: quoted-printable Message-Id: <329C4753-BB97-4C67-8CDA-39EB67E16CE8@freebsd.org> References: <202011150748.0AF7mqW3016900@repo.freebsd.org> To: Scott Long X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Rspamd-Queue-Id: 4CZ1km5R80z3H94 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Nov 2020 19:01:04 -0000 On 15 Nov 2020, at 18:46, Scott Long wrote: >> On Nov 15, 2020, at 11:31 AM, Jessica Clarke = wrote: >>=20 >> Hi Scott, >> I'm concerned by this diff; see my comments below. >>=20 >>> On 15 Nov 2020, at 07:48, Scott Long wrote: >>>=20 >>> Author: scottl >>> Date: Sun Nov 15 07:48:52 2020 >>> New Revision: 367701 >>> URL: https://svnweb.freebsd.org/changeset/base/367701 >>>=20 >>> 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 >>>=20 >>> Modified: >>> head/lib/libutil/getlocalbase.c >>>=20 >>> Modified: head/lib/libutil/getlocalbase.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- 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; >>>=20 >>> if ((pathlen =3D=3D 0) || (path =3D=3D NULL)) { >>> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen) >>> return (-1); >>> } >>>=20 >>> + /* It's unlikely that the buffer would be this big */ >>> + if (pathlen > SSIZE_MAX) { >>> + errno =3D ENOMEM; >>> + return (-1); >>> + } >>> + >>> tmppath =3D NULL; >>> - tmplen =3D pathlen; >>> + tmplen =3D (size_t)pathlen; >>> if (issetugid() =3D=3D 0) >>> tmppath =3D getenv("LOCALBASE"); >>>=20 >>> if ((tmppath =3D=3D NULL) && >>> - (sysctlbyname("user.localbase", path, &tmplen, NULL, 0) =3D=3D= 0)) { >>> + (sysctlbyname("user.localbase", path, (size_t *)&tmplen, = NULL, >>=20 >> This is dangerous and generally a sign of something wrong. >=20 > I think that the danger was mitigated by the overflow check above, but = I > agree that this is generally a poor pattern. >=20 >>=20 >>> + 0) =3D=3D 0)) { >>> return (tmplen); >>> } >>>=20 >>> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen) >>> #endif >>>=20 >>> tmplen =3D strlcpy(path, tmppath, pathlen); >>> - if ((tmplen < 0) || (tmplen >=3D pathlen)) { >>> + if ((tmplen < 0) || (tmplen >=3D (ssize_t)pathlen)) { >>=20 >> 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. >>=20 >=20 > 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=E2=80=99ve looked at the = implementation > of strlcpy(), I see that you=E2=80=99re correct. The man pages are = definitely > confusing, and this isn=E2=80=99t the only place where I think = there=E2=80=99s > inconsistency in the documentation, or at least poor wording choices. >=20 >> 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. >>=20 >=20 > 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=E2=80=99ll > 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