On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote: > On Sat, May 26, 2018, 1:11 AM Eitan Adler <li...@eitanadler.com> wrote: > > > On 25 May 2018 at 08:23, Marcelo Araujo <araujobsdp...@gmail.com> wrote: > > > > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis <bro...@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
signature.asc
Description: PGP signature
