svn commit: r334199 - head/usr.sbin/bhyve

[Brooks Davis]

On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> 2018-05-26 1:44 GMT+08:00 Brooks Davis <brooks at freebsd.org>:
> 
> > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > On Sat, May 26, 2018, 1:11 AM Eitan Adler <lists at eitanadler.com> wrote:
> > >
> > > > On 25 May 2018 at 08:23, Marcelo Araujo <araujobsdport at gmail.com>
> > wrote:
> > > > >
> > > > >
> > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis <brooks at 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 NULL
> > 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/D15550
> > > > >> >
> > > > >> > Modified:
> > > > >> >   head/usr.sbin/bhyve/bhyverun.c
> > > > >> >
> > > > >> > Modified: head/usr.sbin/bhyve/bhyverun.c
> > > > >> >
> > > > >> >
> > > > ============================================================
> > ==================
> > > > >> > --- 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 = 1, n = 1, s = 1, t = 1;
> > > > >> >       ns = false, scts = false;
> > > > >> >       str = strdup(opt);
> > > > >> > +     assert(str != 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 handle
> > > 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.
> >
> 
> 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 = strdup(opt);
if (str == NULL)
	goto out;

str = strdup(opt);
if (str == NULL)
	err(1, "unable to allocate option memory"); 

> 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+=NDEBUG.  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20180525/1489deea/attachment.sig>