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

Fri May 25 18:57:32 UTC 2018

Thanks Shawn,

I think there are plenty of places to fix this case! Thanks for the extra
work :D.

2018-05-26 2:34 GMT+08:00 Shawn Webb <shawn.webb at hardenedbsd.org>:

> 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
>

-- 

-- 
Marcelo Araujo            (__)araujo at FreeBSD.org
\\\'',)http://www.FreeBSD.org <http://www.freebsd.org/>   \/  \ ^
Power To Server.         .\. /_)