Date: Fri, 25 May 2018 18:21:40 +0000 From: Brooks Davis <brooks@freebsd.org> To: araujo@freebsd.org Cc: Eitan Adler <lists@eitanadler.com>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334199 - head/usr.sbin/bhyve Message-ID: <20180525182139.GE99063@spindle.one-eyed-alien.net> In-Reply-To: <CAOfEmZhm3HPGU3r2nK9o3ry8%2BxpzaFRDCgw3qCKgy2CTOu3NaQ@mail.gmail.com> References: <201805250207.w4P275Pf060725@repo.freebsd.org> <20180525151134.GB99063@spindle.one-eyed-alien.net> <CAOfEmZgV9yssn5v8ZpbkwL=rrifoD1Z=uRxe6a0KyM3mrXrSjQ@mail.gmail.com> <CAF6rxgm32%2B_XazDvbtyFChPigxVB0HQ30r3=CvN65ko=zHq0yA@mail.gmail.com> <CAOfEmZhS4RZPn6%2BqOcU56HUtUgGids79cS=fBKfuGjpBBQQm5g@mail.gmail.com> <20180525174424.GD99063@spindle.one-eyed-alien.net> <CAOfEmZhm3HPGU3r2nK9o3ry8%2BxpzaFRDCgw3qCKgy2CTOu3NaQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--10jrOL3x2xqLmOsH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote: > 2018-05-26 1:44 GMT+08:00 Brooks Davis <brooks@freebsd.org>: >=20 > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote: > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler <lists@eitanadler.com> wrot= e: > > > > > > > On 25 May 2018 at 08:23, Marcelo Araujo <araujobsdport@gmail.com> > > wrote: > > > > > > > > > > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis <brooks@freebsd.org> > > wrote: > > > > >> > > > > >> On Fri, May 25, 2018 at 02:07:05AM +0000, Marcelo Araujo wrote: > > > > >> > Author: araujo > > > > >> > Date: Fri May 25 02:07:05 2018 > > > > >> > New Revision: 334199 > > > > >> > URL: https://svnweb.freebsd.org/changeset/base/334199 > > > > >> > > > > > >> > Log: > > > > >> > Fix a memory leak on topology_parse(). > > > > >> > > > > > >> > strdup(3) allocates memory for a copy of the string, does the > > copy > > > > and > > > > >> > returns a pointer to it. If there is no sufficient memory NU= LL > > is > > > > >> > returned > > > > >> > and the global errno is set to ENOMEM. > > > > >> > We do a sanity check to see if it was possible to allocate > > enough > > > > >> > memory. > > > > >> > > > > > >> > Also as we allocate memory, we need to free this memory used. > > Or it > > > > >> > will > > > > >> > going out of scope leaks the storage it points to. > > > > >> > > > > > >> > Reviewed by: rgrimes > > > > >> > MFC after: 3 weeks. > > > > >> > X-MFC: r332298 > > > > >> > Sponsored by: iXsystems Inc. > > > > >> > Differential Revision: https://reviews.freebsd.org/D155= 50 > > > > >> > > > > > >> > Modified: > > > > >> > head/usr.sbin/bhyve/bhyverun.c > > > > >> > > > > > >> > Modified: head/usr.sbin/bhyve/bhyverun.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/usr.sbin/bhyve/bhyverun.c Fri May 25 01:38:59 2018 > > > > >> > (r334198) > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.c Fri May 25 02:07:05 2018 > > > > >> > (r334199) > > > > >> > @@ -193,6 +193,7 @@ topology_parse(const char *opt) > > > > >> > c =3D 1, n =3D 1, s =3D 1, t =3D 1; > > > > >> > ns =3D false, scts =3D false; > > > > >> > str =3D strdup(opt); > > > > >> > + assert(str !=3D NULL); > > > > >> > > > > >> Using assert seems like an odd choice when you've already added a > > > > >> failure path and the strsep will crash immediately if assert is > > elided. > > > > > > > > > > > > > > > Just to make a better point, I had the same discussion about > > assert(3) in > > > > > another review, we don't do NDEBUG even for RELEASE. > > > > > > > > IMHO we only use assert for asserting things ought to never be false > > > > except in buggy code. Using assert for handling is poor practice. > > > > > > > > > > Again, in this case we are using it all over the place and we must > > replace > > > it. Also we should document it in somewhere perhaps in the assert(3) > > > otherwise myself and others will keep using it. If you use find, not = only > > > myself is using it to check strdup! So what is the suggestion to hand= le > > > assert(3)? Deprecated it? > > > > Code that uses assert() in place of error handling is wrong and should > > be fixed. assert(condition) means that condition must never happen > > and if it does a bug has occurred (or the programmers assumptions are > > wrong). In this case failure would not be due to a bug, but do to > > resource exhaustion which is expected to be handled. > > >=20 > I agree with you! We have plenty of place that use strdup(3) without check > the errno ENOMEN return; so do you think would be better bypass a errno > ENOMEN without check it and have a crash, or better abort(3) using > assert(3) in case we have no memory available to allocated the memory for= a > copy of a string? The correct code here would be one of: str =3D strdup(opt); if (str =3D=3D NULL) goto out; str =3D strdup(opt); if (str =3D=3D NULL) err(1, "unable to allocate option memory");=20 > Personally I don't mind make couple extra lines of code to call abort(3) = or > exit(3), but till there, if we don't make RELEASE using NDEBUG, what you > guys are saying to me is more personal preference than anything else. The fact that we don't do NDEBUG builds normally does not allow us to ignore that it exists. It's perfectly reasonable for a user to build with CFLAGS+=3DNDEBUG. That need to work. If code is going to fail to handle resource errors with NDEBUG set then it needs something like this at the top of the file: #ifdef NDEBUG #error The code depends on assert() for error handling #endif -- Brooks --10jrOL3x2xqLmOsH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJbCFQzAAoJEKzQXbSebgfAZjUH/1fsjztyfjyvKItQDy1QmnP3 UjFPO9CudOqzGDNgwgR4m50lsFsetHr67ylexujNliBgPCKG7EgGS+Jn4G34jvuM VbfRnRYxb9WqBdBDejUuXgJSNENsCvciDXUD8/gzefXlCzimBhonxxgKWyfBuDMT PNH4fA6xvL8noUfy1MCz2Culo4oF2IDPKblRAAC0SpzRUSs5txyvAevzqBD3Xsps bByvAV5lKpUwnCblm5qfSS0UUONMTmmc1k5T2MFf+OZo3XcED3mjCLaLF8VA0w06 FPsHfQgktJnisQwf5eFszJtAt3dK/HAiv/FvXZNaRpY+BnS+3gDNgZ7Cj/2M5zk= =B59V -----END PGP SIGNATURE----- --10jrOL3x2xqLmOsH--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180525182139.GE99063>