Date: Mon, 1 Feb 2016 23:59:57 +0000 From: Brooks Davis <brooks@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Mike Belopuhov <mike@belopuhov.com>, "freebsd-arch@freebsd.org" <arch@freebsd.org>, Ryan Stone <rysto32@gmail.com> Subject: Re: OpenBSD mallocarray Message-ID: <20160201235957.GC1747@spindle.one-eyed-alien.net> In-Reply-To: <CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA@mail.gmail.com> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAG6CVpWbaFOQ1GzE1qmZFodXg_xZafmCc0b1kUh=0%2BFAjLPRvA@mail.gmail.com> <CAFMmRNyNKOgDEY89dVB=dqYDq6XyQo=MQR%2BHPJ2=_0VdDKRvAw@mail.gmail.com> <20160201210256.GA29188@yamori.belopuhov.com> <1EA0ECF5-D7AC-430E-957D-C4D49F9A872B@bsdimp.com> <20160201224854.GB1747@spindle.one-eyed-alien.net> <CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--ADZbWkCsHQ7r3kzd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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@freebsd.org> wrote: >=20 > > On Mon, Feb 01, 2016 at 02:12:20PM -0700, Warner Losh wrote: > > > > > > > On Feb 1, 2016, at 2:02 PM, Mike Belopuhov <mike@belopuhov.com> wro= te: > > > > > > > > On Mon, Feb 01, 2016 at 15:56 -0500, Ryan Stone wrote: > > > >> On Mon, Feb 1, 2016 at 3:16 PM, Conrad Meyer <cem@freebsd.org> wro= te: > > > >> > > > >>> > > > >>> Sure. +1 from me. I don't think we want the M_CANFAIL hack, tho= ugh. > > > >>> > > > >>> 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 overfl= ows > > 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. > > >=20 > 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 --ADZbWkCsHQ7r3kzd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJWr/F8AAoJEKzQXbSebgfAJacH/RJ0Rvp/1/JRqguNmnsJgJ1e TzWHmqeir6d1aN2jEQ+bDWvVSAp8v7SB074o7cuz6dX6neyA5O1+qeTwCNFbemM7 6Ii5JB1UVOlTA0VCb9FM8CFhw816bg94phGL+RGLOO499P3uUq2ybVeSDD8ns54x D+0jifE+WGs94zdZhcFMr+CQIVqKGD8yGVa4xj1cgJLD6hOUlw/yxjfvXD1ZLezL y15H6F0hRC98lrBYRCaDNLEqhpLvBH5zq5B+cuADQzmpJYk40HrsSypLmXlTyrWF zpb9elq93ohk73uGdo5jXeafoJGiWSQtSKW7C1Gp3AZsMleHanrM0dWIVk/vSgg= =gN2r -----END PGP SIGNATURE----- --ADZbWkCsHQ7r3kzd--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160201235957.GC1747>