OpenBSD mallocarray

[Brooks Davis]

On Mon, Feb 01, 2016 at 04:01:14PM -0700, Warner Losh wrote:
> On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis <brooks at freebsd.org> wrote:
> 
> > On Mon, Feb 01, 2016 at 02:12:20PM -0700, Warner Losh wrote:
> > >
> > > > On Feb 1, 2016, at 2:02 PM, Mike Belopuhov <mike at belopuhov.com> wrote:
> > > >
> > > > On Mon, Feb 01, 2016 at 15:56 -0500, Ryan Stone wrote:
> > > >> On Mon, Feb 1, 2016 at 3:16 PM, Conrad Meyer <cem at freebsd.org> wrote:
> > > >>
> > > >>>
> > > >>> Sure.  +1 from me.  I don't think we want the M_CANFAIL hack, though.
> > > >>>
> > > >>> Best,
> > > >>> Conrad
> > > >>>
> > > >>>
> > > >> That may be the OpenBSD equivalent of M_NOWAIT.
> > > >
> > > > Not quite.  From the man page:
> > > >
> > > >   M_CANFAIL
> > > >
> > > >   In the M_WAITOK case, if not enough memory is available,
> > > >   return NULL instead of calling panic(9).  If mallocarray()
> > > >   detects an overflow or malloc() detects an excessive
> > > >   allocation, return NULL instead of calling panic(9).
> > >
> > > Yea, we don???t want it calling panic. Ever. That turns an overflow
> > > into a DoS. Arguments should be properly checked so we can
> > > properly return EINVAL for bat-**** crazy ones. FreeBSD???s malloc
> > > doesn???t cave an excessive detector in it.
> > >
> > > My concern with this is that we have a number of different allocation
> > > routines in FreeBSD. This only goes after the malloc() vector, and
> > > even then it requires code changes.
> > >
> > > At best, CANFAIL is a kludge to fail with a panic instead of an
> > > overflow. That???s got to be at most a transient thing until all the
> > > code that it is kludged into with out proper thought is fixed. I???m not
> > > sure that???s something that we want to encourage. I???m all for
> > > safety, but this flag seems both unsafe and unwise.
> >
> > Given that current code could be doing literally anything in the
> > overflow case (and thanks to modern undefined behavior optimizations is
> > likely doing something extraordinarily bizarre), I think turning overflows
> > into panics is a good thing.  Yes, you can argue that means you've added
> > a DoS vector, but best case you had an under allocation and bizarre
> > memory corruption before.  If the default or even only behavior is going
> > to be that overflow fails then we need a static checker that ensure we
> > check the return value even in the M_WAITOK.  Otherwise there will be
> > blind conversions of malloc to mallocarray that go unchecked.
> >
> 
> Returning NULL should be sufficient. Blind conversion of malloc to
> mallocarray in the kernel is also stupid. Intelligent conversion is
> needed to ensure that the error conditions are handled correctly.
> There's no need for a flag to say 'I am going to do the right thing
> if you give me NULL back'. The conversion should do the right
> thing when you get NULL back. A quick survey of the current kernel
> shows that there's not very many that could be using user defined
> values, but does show a large number of places where we could
> use this API.

On further consideration, I think returning NULL is sufficient since most
of the time failure to check will result in a nearby fault.  The main
thing is eliminating the undefined behavior.

-- Brooks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20160201/aa187cc5/attachment.sig>