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

[Marcelo Araujo]

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.


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

Yes, this one makes sense.


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

Please document it in some place!



>
> #ifdef NDEBUG
> #error The code depends on assert() for error handling
> #endif
>

I have no comments for this part!


>
> -- Brooks
>



-- 

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