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

[Rodney W. Grimes]

> On Sat, May 26, 2018, 4:22 AM Rodney W. Grimes <
> free...@pdx.rh.cn85.dnsmgr.net> wrote:
> 
> > > On Sat, May 26, 2018, 4:09 AM Warner Losh  wrote:
> > >
> > > >
> > > >
> > > > On Fri, May 25, 2018 at 2:02 PM, Ed Maste  wrote:
> > > >
> > > >> On 25 May 2018 at 14:26, Marcelo Araujo 
> > wrote:
> > > >> >
> > > >> >> 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!
> > > >>
> > > >> NDEBUG is documented in assert(3). The man page should have more of an
> > > >> explanation (and examples) of the possible pitfalls of assert()
> > > >> though
> > > >>
> > > >
> > > > NDEBUG has been documented in the assert man page since it entered Unix
> > > > via PBW in the 7th Edition Unix from Bell Labs. It's part of the C
> > > > standard, as well as many POSIX and SVID docs.
> > > >
> > >
> > > Yes I can read that! Now tell me, do we build FreeBSD without assert?
> > >
> > > If we do, probably we can't run it without crash!
> >
> > So that makes it perfectly fine to continue what is a well known bad
> > practice?  I do not think so.
> >
> > Many people have tried to persuade you that the *proper* way to check
> > the return from a function is with an if statement, not with an assert,
> > please try to accept that this is pretty much standard accepted portable
> > 'C' coding, and realize all those places you see assert(foo) checking
> > the return of a function are more than likely lurking bugs to be fixed.
> >
> 
> I never said that I didn't accepted that!

You flat out rejected it, more than once, and from more than one source.

> What I have been saying the issue
> is all around and we need to fix it.

You never said we need to fix any of the asserts until prehaps just now.

> Please don't twist my words!

I did not twist your words.

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

On Sat, May 26, 2018, 4:22 AM Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:

> > On Sat, May 26, 2018, 4:09 AM Warner Losh  wrote:
> >
> > >
> > >
> > > On Fri, May 25, 2018 at 2:02 PM, Ed Maste  wrote:
> > >
> > >> On 25 May 2018 at 14:26, Marcelo Araujo 
> wrote:
> > >> >
> > >> >> 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!
> > >>
> > >> NDEBUG is documented in assert(3). The man page should have more of an
> > >> explanation (and examples) of the possible pitfalls of assert()
> > >> though
> > >>
> > >
> > > NDEBUG has been documented in the assert man page since it entered Unix
> > > via PBW in the 7th Edition Unix from Bell Labs. It's part of the C
> > > standard, as well as many POSIX and SVID docs.
> > >
> >
> > Yes I can read that! Now tell me, do we build FreeBSD without assert?
> >
> > If we do, probably we can't run it without crash!
>
> So that makes it perfectly fine to continue what is a well known bad
> practice?  I do not think so.
>
> Many people have tried to persuade you that the *proper* way to check
> the return from a function is with an if statement, not with an assert,
> please try to accept that this is pretty much standard accepted portable
> 'C' coding, and realize all those places you see assert(foo) checking
> the return of a function are more than likely lurking bugs to be fixed.
>

I never said that I didn't accepted that! What I have been saying the issue
is all around and we need to fix it. Please don't twist my words!

Best,

>
> --
> Rod Grimes
> rgri...@freebsd.org
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Rodney W. Grimes]

> On Sat, May 26, 2018, 4:09 AM Warner Losh  wrote:
> 
> >
> >
> > On Fri, May 25, 2018 at 2:02 PM, Ed Maste  wrote:
> >
> >> On 25 May 2018 at 14:26, Marcelo Araujo  wrote:
> >> >
> >> >> 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!
> >>
> >> NDEBUG is documented in assert(3). The man page should have more of an
> >> explanation (and examples) of the possible pitfalls of assert()
> >> though
> >>
> >
> > NDEBUG has been documented in the assert man page since it entered Unix
> > via PBW in the 7th Edition Unix from Bell Labs. It's part of the C
> > standard, as well as many POSIX and SVID docs.
> >
> 
> Yes I can read that! Now tell me, do we build FreeBSD without assert?
> 
> If we do, probably we can't run it without crash!

So that makes it perfectly fine to continue what is a well known bad
practice?  I do not think so.  

Many people have tried to persuade you that the *proper* way to check
the return from a function is with an if statement, not with an assert,
please try to accept that this is pretty much standard accepted portable
'C' coding, and realize all those places you see assert(foo) checking
the return of a function are more than likely lurking bugs to be fixed.

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

On Sat, May 26, 2018, 4:09 AM Warner Losh  wrote:

>
>
> On Fri, May 25, 2018 at 2:02 PM, Ed Maste  wrote:
>
>> On 25 May 2018 at 14:26, Marcelo Araujo  wrote:
>> >
>> >> 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!
>>
>> NDEBUG is documented in assert(3). The man page should have more of an
>> explanation (and examples) of the possible pitfalls of assert()
>> though
>>
>
> NDEBUG has been documented in the assert man page since it entered Unix
> via PBW in the 7th Edition Unix from Bell Labs. It's part of the C
> standard, as well as many POSIX and SVID docs.
>

Yes I can read that! Now tell me, do we build FreeBSD without assert?

If we do, probably we can't run it without crash!


> Warner
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Edward Tomasz Napierała]

On 0526T0226, Marcelo Araujo wrote:
> 2018-05-26 2:21 GMT+08:00 Brooks Davis :

[..]

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

FWIW, calling free(3) on a NULL pointer is perfectly fine.

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Warner Losh]

On Fri, May 25, 2018 at 2:02 PM, Ed Maste  wrote:

> On 25 May 2018 at 14:26, Marcelo Araujo  wrote:
> >
> >> 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!
>
> NDEBUG is documented in assert(3). The man page should have more of an
> explanation (and examples) of the possible pitfalls of assert()
> though
>

NDEBUG has been documented in the assert man page since it entered Unix via
PBW in the 7th Edition Unix from Bell Labs. It's part of the C standard, as
well as many POSIX and SVID docs.

Warner
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Ed Maste]

On 25 May 2018 at 14:26, Marcelo Araujo  wrote:
>
>> 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!

NDEBUG is documented in assert(3). The man page should have more of an
explanation (and examples) of the possible pitfalls of assert()
though.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Ed Maste]

On 25 May 2018 at 14:29, Marcelo Araujo  wrote:
>
> One more thing, exit with err(1) is wrong, 1 is EPERM and should be 12
> ENOMEN! :D

No, please see the err(3) manpage - err's first argument is the exit
code for the program, not an errno.

(err suggests using exit codes from sysexits(3), but there is much
disagreement about that. Either way it's not an errno.)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

2018-05-26 3:01 GMT+08:00 Shawn Webb :

> On Sat, May 26, 2018 at 02:57:29AM +0800, Marcelo Araujo wrote:
> > Thanks Shawn,
> >
> > I think there are plenty of places to fix this case! Thanks for the extra
> > work :D.
>
> Any time. I'm glad to help. If you'd like, I might have time on Sunday
> to audit bhyve's code to find and fix more of these cases.
>

Doesn't hurt and I think it is very welcome!

Best,


>
> Thanks,
>
> --
> Shawn Webb
> Cofounder and Security Engineer
> HardenedBSD
>
> Tor-ified Signal:+1 443-546-8752
> Tor+XMPP+OTR:latt...@is.a.hacker.sx
> GPG Key ID:  0x6A84658F52456EEE
> GPG Key Fingerprint: 2ABA B6BD EF6A F486 BE89  3D9E 6A84 658F 5245 6EEE
>



-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Shawn Webb]

On Sat, May 26, 2018 at 02:57:29AM +0800, Marcelo Araujo wrote:
> Thanks Shawn,
> 
> I think there are plenty of places to fix this case! Thanks for the extra
> work :D.

Any time. I'm glad to help. If you'd like, I might have time on Sunday
to audit bhyve's code to find and fix more of these cases.

Thanks,

-- 
Shawn Webb
Cofounder and Security Engineer
HardenedBSD

Tor-ified Signal:+1 443-546-8752
Tor+XMPP+OTR:latt...@is.a.hacker.sx
GPG Key ID:  0x6A84658F52456EEE
GPG Key Fingerprint: 2ABA B6BD EF6A F486 BE89  3D9E 6A84 658F 5245 6EEE


signature.asc
Description: PGP signature

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

[Marcelo Araujo]

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 :

> On Sat, May 26, 2018 at 02:26:33AM +0800, Marcelo Araujo wrote:
> > 2018-05-26 2:21 GMT+08:00 Brooks Davis :
> >
> > > On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> > > > 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> > > >
> > > > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler 
> > > 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 +, 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.cFri May 25 01:38:59
> 2018
> > > > > > > >> > (r334198)
> > > > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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, 

Re: 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 :
> > 
> > > On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> > > > 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> > > >
> > > > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler 
> > > wrote:
> > > > > >
> > > > > > > On 25 May 2018 at 08:23, Marcelo Araujo 
> > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> > > > > wrote:
> > > > > > > >>
> > > > > > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 
> > > > > > > >> > 2018
> > > > > > > >> > (r334198)
> > > > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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 

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

[Shawn Webb]

On Sat, May 26, 2018 at 02:26:33AM +0800, Marcelo Araujo wrote:
> 2018-05-26 2:21 GMT+08:00 Brooks Davis :
> 
> > On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> > > 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> > >
> > > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler 
> > wrote:
> > > > >
> > > > > > On 25 May 2018 at 08:23, Marcelo Araujo 
> > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> > > > wrote:
> > > > > > >>
> > > > > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > > > > > >> > (r334198)
> > > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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 

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

[Marcelo Araujo]

2018-05-26 2:21 GMT+08:00 Brooks Davis :

> On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> > 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> >
> > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler 
> wrote:
> > > >
> > > > > On 25 May 2018 at 08:23, Marcelo Araujo 
> > > wrote:
> > > > > >
> > > > > >
> > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> > > wrote:
> > > > > >>
> > > > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > > > > >> > (r334198)
> > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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;
>
> str = strdup(opt);
> if (str == NULL)
> err(1, "unable to allocate option memory");
>


One more thing, exit with err(1) is wrong, 1 is EPERM and should be 12
ENOMEN! :D


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

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

[Marcelo Araujo]

2018-05-26 2:21 GMT+08:00 Brooks Davis :

> On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> > 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> >
> > > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > > On Sat, May 26, 2018, 1:11 AM Eitan Adler 
> wrote:
> > > >
> > > > > On 25 May 2018 at 08:23, Marcelo Araujo 
> > > wrote:
> > > > > >
> > > > > >
> > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> > > wrote:
> > > > > >>
> > > > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > > > > >> > (r334198)
> > > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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 

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

[Brooks Davis]

On Sat, May 26, 2018 at 01:56:28AM +0800, Marcelo Araujo wrote:
> 2018-05-26 1:44 GMT+08:00 Brooks Davis :
> 
> > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > > On Sat, May 26, 2018, 1:11 AM Eitan Adler  wrote:
> > >
> > > > On 25 May 2018 at 08:23, Marcelo Araujo 
> > wrote:
> > > > >
> > > > >
> > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> > wrote:
> > > > >>
> > > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > > > >> > (r334198)
> > > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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;

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

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

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

-- Brooks


signature.asc
Description: PGP signature

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

[Marcelo Araujo]

2018-05-26 1:44 GMT+08:00 Brooks Davis :

> On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> > On Sat, May 26, 2018, 1:11 AM Eitan Adler  wrote:
> >
> > > On 25 May 2018 at 08:23, Marcelo Araujo 
> wrote:
> > > >
> > > >
> > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis 
> wrote:
> > > >>
> > > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > > >> > (r334198)
> > > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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?

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.


>
> -- Brooks
>



-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Brooks Davis]

On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote:
> On Sat, May 26, 2018, 1:11 AM Eitan Adler  wrote:
> 
> > On 25 May 2018 at 08:23, Marcelo Araujo  wrote:
> > >
> > >
> > > On Fri, May 25, 2018, 11:11 PM Brooks Davis  wrote:
> > >>
> > >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> > >> > (r334198)
> > >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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

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

[Marcelo Araujo]

On Sat, May 26, 2018, 1:11 AM Eitan Adler  wrote:

> On 25 May 2018 at 08:23, Marcelo Araujo  wrote:
> >
> >
> > On Fri, May 25, 2018, 11:11 PM Brooks Davis  wrote:
> >>
> >> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> >> > (r334198)
> >> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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?

>
>
>
>
> --
> Eitan Adler
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Eitan Adler]

On 25 May 2018 at 08:23, Marcelo Araujo  wrote:
>
>
> On Fri, May 25, 2018, 11:11 PM Brooks Davis  wrote:
>>
>> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
>> > (r334198)
>> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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.



-- 
Eitan Adler
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

On Fri, May 25, 2018, 11:11 PM Brooks Davis  wrote:

> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> (r334198)
> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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.

>
> -- Brooks
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

On Fri, May 25, 2018, 11:11 PM Brooks Davis  wrote:

> On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> (r334198)
> > +++ head/usr.sbin/bhyve/bhyverun.cFri 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.
>

Why assert is an odd choice?  Have a better suggestion?

>
> -- Brooks
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

[Brooks Davis]

On Fri, May 25, 2018 at 02:07:05AM +, 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.cFri May 25 01:38:59 2018
> (r334198)
> +++ head/usr.sbin/bhyve/bhyverun.cFri 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.

-- Brooks


signature.asc
Description: PGP signature

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

[Marcelo Araujo]

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);
 
while ((cp = strsep(, ",")) != NULL) {
if (sscanf(cp, "%i%n", , ) == 1) {
@@ -218,11 +219,13 @@ topology_parse(const char *opt)
} else if (cp[0] == '\0')
continue;
else
-   return (-1);
+   goto out;
/* Any trailing garbage causes an error */
if (cp[chk] != '\0')
-   return (-1);
+   goto out;
}
+   free(str);
+
/*
 * Range check 1 <= n <= UINT16_MAX all values
 */
@@ -248,6 +251,10 @@ topology_parse(const char *opt)
cores = c;
threads = t;
return(0);
+
+out:
+   free(str);
+   return (-1);
 }
 
 static int
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"