From owner-svn-src-all@freebsd.org Sun Nov 15 18:46:38 2020 Return-Path: Delivered-To: svn-src-all@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 F20C446414B; Sun, 15 Nov 2020 18:46:38 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4CZ1Q65npkz3G5F; Sun, 15 Nov 2020 18:46:38 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.west.internal (Postfix) with ESMTP id 3D975D3D; Sun, 15 Nov 2020 13:46:37 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Sun, 15 Nov 2020 13:46:37 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; s=fm1; bh=v +KQUI76kLBZ2DxWgzRrqAsg3oDGrYCIrdO9xCASbeE=; b=Rwwjwm5ScTXqVzqor 41WhbAGVR+Jurpw1tVYGc5jg0BQ4wFe9S0koNws4GO0K+HGHoB3Sr8/dXsskisX8 VhX9eftOVNHsyfUZk4KlidU9udMiqxR2KCJhnLyScfdKa1LhBffwILVzw9F40p6v YOdq+cAhAocjFXy6MHyfKsnfYk7H2rxGDJGxwKi97ezJHUY5Q28nmOv6rldKuOZb EPNmWj+VsDXhfTLY6dsAUuZXG4cPz3P3nreJnEQWmovNibbkzC0aAL/frGp9JR4f 7Zb2iPADNFK7fbAxUWi2bAsZxGBmZWPvUsNDmYju6NFu6mEZzLZ+FEXZz0v0Rpxq U7Q7A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=v+KQUI76kLBZ2DxWgzRrqAsg3oDGrYCIrdO9xCASb eE=; b=DT/+ag9jXoNA/YQiNIU30d2eqt+avePi0/ytG8y6MszUg6JjoMUbnEkFE dbVnfKM8AtRQslmB8I2+aVKVQh73mkZnwSDg+SLGvxMmH8YLJkN5Hws1hXtuVdYJ OLtr49Un07fqsThHAxhRdF60yDeX4AkVStPliZBexKN1HXCpcMtctlZO1IWdoOBa EKmYyBVffePlVcXRmtAiitvgOODQZm67vMcCNKX1b0RtCG97wpxN1WLRYSzLB4cr S7ui7RYHKoPBdzZSvqvU0EtVpeR9WPjikMWg8gu/RPa9jFX7OzYdqkaMxVp3jpaa wDq7zcdwWDevWPm0+5NEjXCbzUAbg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddvledguddvtdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecuufhorhhtvgguuchrvggtihhpshculdegtddmne cujfgurheptggguffhjgffgffkfhfvofesthhqmhdthhdtjeenucfhrhhomhepufgtohht thcunfhonhhguceoshgtohhtthhlsehsrghmshgtohdrohhrgheqnecuggftrfgrthhtvg hrnhepudduveekheehiedukeekleelvedufeevfeetudfgtdffteffleehheffueffgfeh necuffhomhgrihhnpehfrhgvvggsshgurdhorhhgnecukfhppeekrdegiedrkeelrddvud efnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepshgt ohhtthhlsehsrghmshgtohdrohhrgh X-ME-Proxy: Received: from [192.168.0.114] (unknown [8.46.89.213]) by mail.messagingengine.com (Postfix) with ESMTPA id 53AC63064AA7; Sun, 15 Nov 2020 13:46:36 -0500 (EST) 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: Scott Long In-Reply-To: Date: Sun, 15 Nov 2020 11:46:35 -0700 Cc: Scott Long , src-committers , svn-src-all , svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <202011150748.0AF7mqW3016900@repo.freebsd.org> To: Jessica Clarke X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Rspamd-Queue-Id: 4CZ1Q65npkz3G5F 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-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Nov 2020 18:46:39 -0000 > 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. I think that the danger was mitigated by the overflow check above, but I agree that this is generally a poor pattern. >=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 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. > 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 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. Scott