From nobody Wed Apr 24 00:21:45 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 4VPKRp5qjpz5JNRy for ; Wed, 24 Apr 2024 00:21:58 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (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 4VPKRp1VFZz40Ts for ; Wed, 24 Apr 2024 00:21:58 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-41af6701806so2359285e9.1 for ; Tue, 23 Apr 2024 17:21:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713918116; x=1714522916; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hLqaLJommJyVlke3fhNCKj4COU1kjDI1tkHaz/HPaGg=; b=ZV7uO1DTAJjYiXwWaO+ECygCIVHhRdPnn8swjyHdu/NmHpeVnLCmJHPxDBDAB9y2T5 NPDoe7wZaqQqmJSAbOZ7PTwRCoqvU5sp6Mhxg4n6eDAy2799YEELmKfe9dAlt2JhFYE4 ByAJ9xMlK9AJ6irFpA6wr6hMIUub4eibRnE9foPEVelwt1OOze3wLZSZsk3h8ztHpNvm aox5NKA9sQelseQvKNrIIQpP903s8RiSqXcTUX3Z36l+gHKXCqg6ToCXdKPDwEb4Aqw8 f2g7m0bgXcVsr4Tj+zgj9kyn4kH+VNV7uQfhXqJ/hkaT+V9js7gRMS4V3HHDc7BTPipf iiPQ== X-Forwarded-Encrypted: i=1; AJvYcCWjyku2neuT7PtW3IZwGMmm5M6wcH2M0sMzDh6uRgch56L89HRoi4hBINw/6LBc/6Ez3uEWM4hOTh3QYXwj+yy9w2VwmNiJNyVBq7OX0r5A X-Gm-Message-State: AOJu0YxfrFIsSzH0aq5IDWj6j2RH30BD1qGOEoRopZYJiSmonCSRkp+y bsKHwSy/TQ6+6nFrOps6xypywimjstn+vF0Dwpj0saWNllzQfamtEui5TXpzVAk= X-Google-Smtp-Source: AGHT+IHMJttjP1sLZFVC3JHlZ7nbBM3yJvV/rmuab6fgpBE1cgXINjtVKWPV9s9tgZWcQhWI7SbIJw== X-Received: by 2002:a05:600c:a005:b0:41a:1d3a:3fc1 with SMTP id jg5-20020a05600ca00500b0041a1d3a3fc1mr574770wmb.3.1713918116595; Tue, 23 Apr 2024 17:21:56 -0700 (PDT) Received: from smtpclient.apple ([131.111.5.246]) by smtp.gmail.com with ESMTPSA id w14-20020adfee4e000000b00343d6c7240fsm15597785wro.35.2024.04.23.17.21.55 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 23 Apr 2024 17:21:56 -0700 (PDT) Content-Type: text/plain; charset=utf-8 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 (Mac OS X Mail 16.0 \(3774.200.91.1.1\)) Subject: Re: git: 91bdebc958bb - main - bsdinstall/distfetch.c: check environment variables before downloading and handle memory allocation errors From: Jessica Clarke In-Reply-To: <202404232242.43NMgqxx082026@gitrepo.freebsd.org> Date: Wed, 24 Apr 2024 01:21:45 +0100 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <9C180198-A6B6-41D3-AB49-D9582D356ADE@freebsd.org> References: <202404232242.43NMgqxx082026@gitrepo.freebsd.org> To: Warner Losh X-Mailer: Apple Mail (2.3774.200.91.1.1) 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:209.85.128.0/17, country:US] X-Rspamd-Queue-Id: 4VPKRp1VFZz40Ts On 23 Apr 2024, at 23:42, Warner Losh wrote: >=20 > The branch main has been updated by imp: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3D91bdebc958bb0da03f604bad19f99e3b= 10e96ac7 >=20 > commit 91bdebc958bb0da03f604bad19f99e3b10e96ac7 > Author: rilysh > AuthorDate: 2024-04-23 22:40:19 +0000 > Commit: Warner Losh > CommitDate: 2024-04-23 22:42:38 +0000 >=20 > bsdinstall/distfetch.c: check environment variables before = downloading and handle memory allocation errors >=20 > 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. >=20 > 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(-) >=20 > 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; >=20 > - if (getenv("DISTRIBUTIONS") =3D=3D NULL) > + if ((dists =3D getenv("DISTRIBUTIONS")) =3D=3D NULL) > errx(EXIT_FAILURE, "DISTRIBUTIONS variable is not set"); >=20 > - 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 */ >=20 > - 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"); >=20 > 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 = pedantic and free memory prior to calling errx, fine, but do it in full, don=E2=80=99= t 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). > } >=20 > - 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();