From owner-svn-src-all@freebsd.org Sun Nov 15 18:32:03 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 A1706463B55 for ; Sun, 15 Nov 2020 18:32:03 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (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 4CZ15H42Kkz3F8r for ; Sun, 15 Nov 2020 18:32:03 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wm1-f41.google.com with SMTP id h2so21816891wmm.0 for ; Sun, 15 Nov 2020 10:32:03 -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=XWvLvKZg0VA868mq218JtfPiGHbyzwuV7k8i+tTJ+/o=; b=Oo+Z7UlT6psBmeVbiVezvfAGFwfVv7ZNWm911MKh/ZoG9F3X8MCsigGmcfF+aca9/2 7mISM+6u9jf9ndZcF+yPBG2/Z91tDhojHjaTuK6m9qKWjQcIf5vghTPsdaN3r3oVardU YhAFMfODQjVz2Nuxk8RvZXP5POm3EaQyWlqIPITQZrdvSdPMnKYwFEIfxlxtbfjD5Ls4 epPh7gY6MiHMpZxRg3jUI9hp2m9mRpr1q3SQnzfKKsFt6hro5EHHtuFCJUOTRr5FxODw hdsvyUwRmdatzvn0oPVp4EKWgUgQMrjse6R8AMzf4Y6WcXJ6E13Dl06ri7pSSfw84zdr 0Yjw== X-Gm-Message-State: AOAM533dFerovSoZ7cgibRTTKwncJ9IuC2caH0KXpDD51W9SRAjIU2lM uYHh83y0ucpcQa775AsqiiyxbQ== X-Google-Smtp-Source: ABdhPJx+zhTO/PdA7wpUPFD0t9y5Pmmme3Wq5oGZnTf7kd4kLFV4XhN2H+NH1IHhvgrRDgLrTDwTkQ== X-Received: by 2002:a7b:c458:: with SMTP id l24mr11322094wmi.136.1605465121604; Sun, 15 Nov 2020 10:32:01 -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 u5sm16181839wml.13.2020.11.15.10.32.00 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 15 Nov 2020 10:32:00 -0800 (PST) Content-Type: text/plain; charset=us-ascii 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: <202011150748.0AF7mqW3016900@repo.freebsd.org> Date: Sun, 15 Nov 2020 18:31:59 +0000 Cc: src-committers , svn-src-all , svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <202011150748.0AF7mqW3016900@repo.freebsd.org> To: Scott Long X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Rspamd-Queue-Id: 4CZ15H42Kkz3F8r 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:32:03 -0000 Hi Scott, I'm concerned by this diff; see my comments below. > 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, This is dangerous and generally a sign of something wrong. > + 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)) { 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