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

Shawn Webb shawn.webb at hardenedbsd.org
Fri May 25 18:34:25 UTC 2018


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,

-- 
Shawn Webb
Cofounder and Security Engineer
HardenedBSD

Tor-ified Signal:    +1 443-546-8752
Tor+XMPP+OTR:        lattera at is.a.hacker.sx
GPG Key ID:          0x6A84658F52456EEE
GPG Key Fingerprint: 2ABA B6BD EF6A F486 BE89  3D9E 6A84 658F 5245 6EEE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20180525/5410fd38/attachment.sig>


More information about the svn-src-head mailing list