Date: Wed, 24 Apr 2024 01:21:45 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Warner Losh <imp@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 91bdebc958bb - main - bsdinstall/distfetch.c: check environment variables before downloading and handle memory allocation errors Message-ID: <9C180198-A6B6-41D3-AB49-D9582D356ADE@freebsd.org> In-Reply-To: <202404232242.43NMgqxx082026@gitrepo.freebsd.org> References: <202404232242.43NMgqxx082026@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23 Apr 2024, at 23:42, Warner Losh <imp@FreeBSD.org> 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 <nightquick@proton.me> > AuthorDate: 2024-04-23 22:40:19 +0000 > Commit: Warner Losh <imp@FreeBSD.org> > 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 <nightquick@proton.me> > 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();
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9C180198-A6B6-41D3-AB49-D9582D356ADE>