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

[Brooks Davis]

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.

-- 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/22cf3615/attachment.sig>