From owner-svn-src-all@freebsd.org Fri May 25 18:21:41 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 44EC0EF00E8; Fri, 25 May 2018 18:21:41 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id CBA576D2FD; Fri, 25 May 2018 18:21:40 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 0BB1A5A9F13; Fri, 25 May 2018 18:21:40 +0000 (UTC) Date: Fri, 25 May 2018 18:21:40 +0000 From: Brooks Davis To: araujo@freebsd.org Cc: Eitan Adler , src-committers , 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> References: <201805250207.w4P275Pf060725@repo.freebsd.org> <20180525151134.GB99063@spindle.one-eyed-alien.net> <20180525174424.GD99063@spindle.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="10jrOL3x2xqLmOsH" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 May 2018 18:21:41 -0000 --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 : >=20 > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote: > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler wrot= e: > > > > > > > On 25 May 2018 at 08:23, Marcelo Araujo > > wrote: > > > > > > > > > > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis > > 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--