From owner-svn-src-head@freebsd.org Sun Nov 15 18:32:03 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 975AB463DF3 for ; Sun, 15 Nov 2020 18:32:03 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) (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 4CZ15H3gVxz3FL0 for ; Sun, 15 Nov 2020 18:32:03 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wm1-f46.google.com with SMTP id 19so21790634wmf.1 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=m9tk9bhUyJkBq0RvTiTesUhhviHa/7pdWkCkfDMAEbh3Ov6d88Lr6lOESblzY2bpfJ aL4CBPe8piPQNpE3onGCe0ZibygxBUs1wEFjBQRtSiEX0IB0SIp3eqyPqNJMKn7BuHea QzuulRwMJNdF/D91xEZ8cGqRHD0chEsubm0h2nXEXJttonBPDeN7JOVqsZoC8+g36LtV n1lM7yOTZQUrFxKgCI6KN/Em5F914NppXiBWGGYoOLUX35FtsJT2sqN7YGVdJrn1Jmab vX7KD5etzhqDWSnk3mluoaQGLC0FXGeI1i4eLEhymBMrejI9VjpXLHq/pDHpl6mAS6SR u3HA== X-Gm-Message-State: AOAM530K+smLzHGOjkCeeY7XDobefyjeFz8Er6EsjN2mnHP2eH+EhgWa PCwmBjGI8KB1lR5JBEYDWgyEDA== 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: 4CZ15H3gVxz3FL0 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 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