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

[Rodney W. Grimes]

-- Start of PGP signed section.
> On Sat, May 26, 2018 at 02:26:33AM +0800, Marcelo Araujo wrote:
> > 2018-05-26 2:21 GMT+08:00 Brooks Davis <brooks at freebsd.org>:
> > 
> > > 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;
> > >
> > 
> > No, it is not the correct code! If we go out and free(str) we have nothing
> > to free, because we even didn't allocated memory for str.
> 
> Hey Marcelo,
> 
> I've authored this commit, which fixes the issues Brooks brought up
> (and with which I agree):
> 
> https://github.com/HardenedBSD/hardenedBSD/commit/9c05b8def2c33e3889430cc2f54be0402a257366
> 
> Thanks,

And as the author of the function in question I ALSO agree that this fix
by Shawn should be commited.

Thank you Shawn, only a few hundred more like this in bhyve(8) to fix now....


-- 
Rod Grimes                                                 rgrimes at freebsd.org