From nobody Wed Apr 24 03:28:25 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4VPPbD6ggpz5JfDv for ; Wed, 24 Apr 2024 03:28:40 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com [IPv6:2a00:1450:4864:20::12d]) (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 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4VPPbC3Ff0z4JhN for ; Wed, 24 Apr 2024 03:28:39 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-lf1-x12d.google.com with SMTP id 2adb3069b0e04-51ac9c6599bso6194166e87.1 for ; Tue, 23 Apr 2024 20:28:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1713929317; x=1714534117; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=tLBfZrUot7JTc6M8RhNNq09LLplwrMoPYmhHS2Ae3Wk=; b=kHCiqdDuMxOO9jcqB4WtWpz1NRtYnH8Ug09gVzYlq9zWixvF/TmUaWciZ4qAfTnDk6 w4lloY82hjdEWduFHtXBI/lv7R5M0bBAe52yxVjaH3PNnXcTZur7w25Nwzg58QTLB9Yq lkBovYckPboUbpD6ZqyQvOyZCvTF3xFaVcfZvGA4tQcxCo9UfMt+d4yWMLG4zDucTUs4 YZlwsSgyS+qJ3c93ZJXJ5Gjyl8b00KEhOouxoPCC9CqR2mqqYvmt8uMavjAxrMI43+wT fhkeh4LM3sxDxC58OR4OxY80KRqkQVPqjYgwEOYn+YrsjzMONKLGZR7BngMoiansaGad DP1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713929317; x=1714534117; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=tLBfZrUot7JTc6M8RhNNq09LLplwrMoPYmhHS2Ae3Wk=; b=lHtng01C4SFH8vUtVNXD8v9pjpaB3Hvw3Qn3gyLg1ty+kJxPGf+vK9MDqfHv+AYaNk YJATYREt5QXsx+qkhbGREKUrl4bKgSx0vBzf+0yGReGft1Bv0sicMIRg0dzbu26UfOeO K+tVfR59615l3TQxdCWLlDeGo142HCVUE0X4y3k49bZvPE74WWkXGbWI/0l5oMwo34FX FU8q95JdH9xJl31veGiTgvfTMEFdbP9DrjWnXQjiapu1klak/yaP6p65lcrkadX7z35R fj5+PgLQreTuXDbVH4XpfrPp8yRMst6Y3uhMHs+7bNEC6ArxzzqSUvAc/LD38S9fOW3K Q4Ig== X-Forwarded-Encrypted: i=1; AJvYcCWyjlEfIG+eV36sFr4LstFNpAHPCVHzqz45Zv6T9dZ4iGh4lYOLeFfHQKnqEOQYydFmtyX0p5G5riJQkKaOjfHdrI4B6Zg64SVKuis8GDJt X-Gm-Message-State: AOJu0YxVuiGUs7IfJHgW1sRB/OdEP2QjlxxmEyrt1fgxv6XYkvO3y+EM GSbah9n9WS9TtvOV16dYL+/KMYP0bTwOIRUx2871iYVwPDo84YcuPZGkVS9YbNMQaCiawkp7wMb epmqXXy8CUBuBD/rwVrausk6fLQeFsZeu75h2rQ== X-Google-Smtp-Source: AGHT+IGModitcFOwINgolXN6Y3e2CiOCZf1JUDY+wKRY4xjLeKcrkcPcyVaD1T3H55WgOrRLQAHNTD8tdcNMkOq3pn4= X-Received: by 2002:a05:6512:3d2a:b0:518:dc5b:6f4a with SMTP id d42-20020a0565123d2a00b00518dc5b6f4amr1191546lfv.45.1713929317020; Tue, 23 Apr 2024 20:28:37 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 References: <202404232242.43NMgqxx082026@gitrepo.freebsd.org> <9C180198-A6B6-41D3-AB49-D9582D356ADE@freebsd.org> In-Reply-To: <9C180198-A6B6-41D3-AB49-D9582D356ADE@freebsd.org> From: Warner Losh Date: Tue, 23 Apr 2024 21:28:25 -0600 Message-ID: Subject: Re: git: 91bdebc958bb - main - bsdinstall/distfetch.c: check environment variables before downloading and handle memory allocation errors To: Jessica Clarke Cc: Warner Losh , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Type: multipart/alternative; boundary="0000000000001c939c0616cf44d6" X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Queue-Id: 4VPPbC3Ff0z4JhN --0000000000001c939c0616cf44d6 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I've backed this out as well... I'd thought that the style stuff had been worked out... And I'll leave the pull request closed since our newer guidance would suggest this isn't enough of a win (I've been processing the older ones under the old guidance on this point, but now I'm questioning that)... The cleanups aren't that bad (moduo style), but they don't really move the needle enough and would only be acceptable in the context of other improvements (though even then it might be tough to justify). Warner On Tue, Apr 23, 2024 at 6:21=E2=80=AFPM Jessica Clarke = wrote: > On 23 Apr 2024, at 23:42, Warner Losh wrote: > > > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3D91bdebc958bb0da03f604bad19f99e3= b10e96ac7 > > > > commit 91bdebc958bb0da03f604bad19f99e3b10e96ac7 > > Author: rilysh > > AuthorDate: 2024-04-23 22:40:19 +0000 > > Commit: Warner Losh > > CommitDate: 2024-04-23 22:42:38 +0000 > > > > bsdinstall/distfetch.c: check environment variables before > downloading and handle memory allocation errors > > > > 1. Currently, distfetch checks environment variables existence > > when it will use them or in a case (in chdir()) it doesn't check > > at all. As they are necessary to set before doing anything with > > it, check them, if they set or not, before proceeding any further. > > This also avoids extra cleaning when that environment variable > > isn't set. > > > > 2. Handle memory allocation error in malloc(PATH_MAX) and replace > > (sizeof const char *) with (sizeof char *). Both are similar and > > const doesn't have a size. > > Latter is fine I guess but they=E2=80=99re the same by definition, it=E2= =80=99s churn. > > > 3. Indent the error message a bit in chdir(). > > AKA violate style(9). > > > Signed-off-by: rilysh > > Reviewed by: imp > > Pull Request: https://github.com/freebsd/freebsd-src/pull/1071 > > --- > > usr.sbin/bsdinstall/distfetch/distfetch.c | 33 > +++++++++++++++++++++---------- > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/usr.sbin/bsdinstall/distfetch/distfetch.c > b/usr.sbin/bsdinstall/distfetch/distfetch.c > > index c431e187799d..094929d89ea1 100644 > > --- a/usr.sbin/bsdinstall/distfetch/distfetch.c > > +++ b/usr.sbin/bsdinstall/distfetch/distfetch.c > > @@ -46,7 +46,7 @@ static int fetch_files(int nfiles, char **urls); > > int > > main(void) > > { > > - char *diststring; > > + char *diststring, *dists, *distdir, *distsite; > > char **urls; > > int i; > > int ndists =3D 0; > > @@ -54,17 +54,24 @@ main(void) > > char error[PATH_MAX + 512]; > > struct bsddialog_conf conf; > > > > - if (getenv("DISTRIBUTIONS") =3D=3D NULL) > > + if ((dists =3D getenv("DISTRIBUTIONS")) =3D=3D NULL) > > errx(EXIT_FAILURE, "DISTRIBUTIONS variable is not set"); > > > > - diststring =3D strdup(getenv("DISTRIBUTIONS")); > > + if ((distdir =3D getenv("BSDINSTALL_DISTDIR")) =3D=3D NULL) > > + errx(EXIT_FAILURE, "BSDINSTALL_DISTDIR variable is not set"); > > + > > + if ((distsite =3D getenv("BSDINSTALL_DISTSITE")) =3D=3D NULL) > > + errx(EXIT_FAILURE, "BSDINSTALL_DISTSITE variable is not set"); > > + > > + if ((diststring =3D strdup(dists)) =3D=3D NULL) > > + errx(EXIT_FAILURE, "Error: diststring variable out of memory!"); > > + > > for (i =3D 0; diststring[i] !=3D 0; i++) > > if (isspace(diststring[i]) && !isspace(diststring[i+1])) > > ndists++; > > ndists++; /* Last one */ > > > > - urls =3D calloc(ndists, sizeof(const char *)); > > - if (urls =3D=3D NULL) { > > + if ((urls =3D calloc(ndists, sizeof(char *))) =3D=3D NULL) { > > Moving into the if is pointless. > > > free(diststring); > > errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!"); > > } > > @@ -78,15 +85,21 @@ main(void) > > bsddialog_backtitle(&conf, OSNAME " Installer"); > > > > for (i =3D 0; i < ndists; i++) { > > - urls[i] =3D malloc(PATH_MAX); > > + if ((urls[i] =3D malloc(PATH_MAX)) =3D=3D NULL) { > > Moving into the if is pointless. > > > + free(urls); > > + free(diststring); > > Like I said in the review, reviewing urls and diststring but not the > elements of urls is a stupid halfway house, and it=E2=80=99s all a waste = of > time when you=E2=80=99re about to call errx. If one wants to be super ped= antic > and free memory prior to calling errx, fine, but do it in full, don=E2=80= =99t > do an arbitrary subset. > > > + bsddialog_end(); > > + errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!"); > > + } > > + > > snprintf(urls[i], PATH_MAX, "%s/%s", > > - getenv("BSDINSTALL_DISTSITE"), strsep(&diststring, " \t")); > > + distsite, strsep(&diststring, " \t")); > > Breaks style(9). > > > } > > > > - if (chdir(getenv("BSDINSTALL_DISTDIR")) !=3D 0) { > > + if (chdir(distdir) !=3D 0) { > > snprintf(error, sizeof(error), > > - "Could not change to directory %s: %s\n", > > - getenv("BSDINSTALL_DISTDIR"), strerror(errno)); > > + "Could not change to directory %s: %s\n", > > + distdir, strerror(errno)); > > Breaks style(9). > > Jess > > > conf.title =3D "Error"; > > bsddialog_msgbox(&conf, error, 0, 0); > > bsddialog_end(); > > --0000000000001c939c0616cf44d6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I've backed this out as well... I'd thought t= hat the style stuff had been worked out...

And I&#= 39;ll leave the pull request closed since our newer guidance would suggest = this isn't enough of a win (I've been processing the older ones und= er the old guidance on this point, but now I'm questioning that)... The= cleanups aren't that bad (moduo style), but they don't really move= the needle enough and would only be acceptable in the context of other imp= rovements (though even then it might be tough to justify).

Warner

On Tue, Apr 23, 2024 at 6:21=E2=80=AFPM Jessica Clarke &= lt;jrtc27@freebsd.org> wrote:<= br>
On 23 Apr 2024, = at 23:42, Warner Losh <imp@FreeBSD.org> wrote:
>
> The branch main has been updated by imp:
>
> URL: https://= cgit.FreeBSD.org/src/commit/?id=3D91bdebc958bb0da03f604bad19f99e3b10e96ac7<= /a>
>
> commit 91bdebc958bb0da03f604bad19f99e3b10e96ac7
> Author:=C2=A0 =C2=A0 =C2=A0rilysh <nightquick@proton.me>
> AuthorDate: 2024-04-23 22:40:19 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> CommitDate: 2024-04-23 22:42:38 +0000
>
>=C2=A0 =C2=A0 bsdinstall/distfetch.c: check environment variables befor= e downloading and handle memory allocation errors
>
>=C2=A0 =C2=A0 1. Currently, distfetch checks environment variables exis= tence
>=C2=A0 =C2=A0 when it will use them or in a case (in chdir()) it doesn&= #39;t check
>=C2=A0 =C2=A0 at all. As they are necessary to set before doing anythin= g with
>=C2=A0 =C2=A0 it, check them, if they set or not, before proceeding any= further.
>=C2=A0 =C2=A0 This also avoids extra cleaning when that environment var= iable
>=C2=A0 =C2=A0 isn't set.
>
>=C2=A0 =C2=A0 2. Handle memory allocation error in malloc(PATH_MAX) and= replace
>=C2=A0 =C2=A0 (sizeof const char *) with (sizeof char *). Both are simi= lar and
>=C2=A0 =C2=A0 const doesn't have a size.

Latter is fine I guess but they=E2=80=99re the same by definition, it=E2=80= =99s churn.

>=C2=A0 =C2=A0 3. Indent the error message a bit in chdir().

AKA violate style(9).

>=C2=A0 =C2=A0 Signed-off-by: rilysh <nightquick@proton.me>
>=C2=A0 =C2=A0 Reviewed by: imp
>=C2=A0 =C2=A0 Pull Request: https://github.com/f= reebsd/freebsd-src/pull/1071
> ---
> usr.sbin/bsdinstall/distfetch/distfetch.c | 33 +++++++++++++++++++++--= --------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/usr.sbin/bsdinstall/distfetch/distfetch.c b/usr.sbin/bsdi= nstall/distfetch/distfetch.c
> index c431e187799d..094929d89ea1 100644
> --- a/usr.sbin/bsdinstall/distfetch/distfetch.c
> +++ b/usr.sbin/bsdinstall/distfetch/distfetch.c
> @@ -46,7 +46,7 @@ static int fetch_files(int nfiles, char **urls);
> int
> main(void)
> {
> - char *diststring;
> + char *diststring, *dists, *distdir, *distsite;
> char **urls;
> int i;
> int ndists =3D 0;
> @@ -54,17 +54,24 @@ main(void)
> char error[PATH_MAX + 512];
> struct bsddialog_conf conf;
>
> - if (getenv("DISTRIBUTIONS") =3D=3D NULL)
> + if ((dists =3D getenv("DISTRIBUTIONS")) =3D=3D NULL)
> errx(EXIT_FAILURE, "DISTRIBUTIONS variable is not set");
>
> - diststring =3D strdup(getenv("DISTRIBUTIONS"));
> + if ((distdir =3D getenv("BSDINSTALL_DISTDIR")) =3D=3D NULL= )
> + errx(EXIT_FAILURE, "BSDINSTALL_DISTDIR variable is not set"= ;);
> +
> + if ((distsite =3D getenv("BSDINSTALL_DISTSITE")) =3D=3D NU= LL)
> + errx(EXIT_FAILURE, "BSDINSTALL_DISTSITE variable is not set&quo= t;);
> +
> + if ((diststring =3D strdup(dists)) =3D=3D NULL)
> + errx(EXIT_FAILURE, "Error: diststring variable out of memory!&q= uot;);
> +
> for (i =3D 0; diststring[i] !=3D 0; i++)
> if (isspace(diststring[i]) && !isspace(diststring[i+1]))
> ndists++;
> ndists++; /* Last one */
>
> - urls =3D calloc(ndists, sizeof(const char *));
> - if (urls =3D=3D NULL) {
> + if ((urls =3D calloc(ndists, sizeof(char *))) =3D=3D NULL) {

Moving into the if is pointless.

> free(diststring);
> errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!");<= br> > }
> @@ -78,15 +85,21 @@ main(void)
> bsddialog_backtitle(&conf, OSNAME " Installer");
>
> for (i =3D 0; i < ndists; i++) {
> - urls[i] =3D malloc(PATH_MAX);
> + if ((urls[i] =3D malloc(PATH_MAX)) =3D=3D NULL) {

Moving into the if is pointless.

> + free(urls);
> + free(diststring);

Like I said in the review, reviewing urls and diststring but not the
elements of urls is a stupid halfway house, and it=E2=80=99s all a waste of=
time when you=E2=80=99re about to call errx. If one wants to be super pedan= tic
and free memory prior to calling errx, fine, but do it in full, don=E2=80= =99t
do an arbitrary subset.

> + bsddialog_end();
> + errx(EXIT_FAILURE, "Error: distfetch URLs out of memory!")= ;
> + }
> +
> snprintf(urls[i], PATH_MAX, "%s/%s",
> -=C2=A0 =C2=A0 getenv("BSDINSTALL_DISTSITE"), strsep(&di= ststring, " \t"));
> + distsite, strsep(&diststring, " \t"));

Breaks style(9).

> }
>
> - if (chdir(getenv("BSDINSTALL_DISTDIR")) !=3D 0) {
> + if (chdir(distdir) !=3D 0) {
> snprintf(error, sizeof(error),
> -=C2=A0 =C2=A0 "Could not change to directory %s: %s\n",
> -=C2=A0 =C2=A0 getenv("BSDINSTALL_DISTDIR"), strerror(errno)= );
> + "Could not change to directory %s: %s\n",
> + distdir, strerror(errno));

Breaks style(9).

Jess

> conf.title =3D "Error";
> bsddialog_msgbox(&conf, error, 0, 0);
> bsddialog_end();

--0000000000001c939c0616cf44d6--