Date: Mon, 1 Feb 2016 15:49:14 -0700 From: Warner Losh <imp@bsdimp.com> To: cem@freebsd.org Cc: Mike Belopuhov <mike@belopuhov.com>, "freebsd-arch@freebsd.org" <arch@freebsd.org>, Ryan Stone <rysto32@gmail.com> Subject: Re: OpenBSD mallocarray Message-ID: <CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ@mail.gmail.com> In-Reply-To: <CAG6CVpUySF%2BbWKW7xvPMxOnYKs8KntSv0pX%2B=X00Qi7=DNithg@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> <CAG6CVpUySF%2BbWKW7xvPMxOnYKs8KntSv0pX%2B=X00Qi7=DNithg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Feb 1, 2016 at 3:19 PM, Conrad Meyer <cem@freebsd.org> wrote: > On Mon, Feb 1, 2016 at 1:12 PM, Warner Losh <imp@bsdimp.com> wrote: > > > >> On Feb 1, 2016, at 2:02 PM, Mike Belopuhov <mike@belopuhov.com> wrote: > >> 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=E2=80=99t want it calling panic. Ever. That turns an overfl= ow > > into a DoS. > > I disagree. The panic is essentially an assertion that malloc was > passed valid arguments. We have similar invariants assertions > throughout the kernel and it is the only sane thing to do with > overflow + M_WAITOK. M_WAITOK callers today will do something equally > stupid if they get a NULL result from mallocarray(). We only do this for things that can't happen. If the user can trigger this panic by passing some bogus, unchecked value that doesn't get checked until here, that's bad. That's fundamentally different than getting 'freeing free inode' from ufs. Users can't trigger the latter. > > > Arguments should be properly checked > > Yes! That's why the assertion is a good thing. We disagree on this then. This isn't a 'fail safe' this is a 'fail with denial of system'. that' > > At best, CANFAIL is a kludge to fail with a panic instead of an > > overflow. > > No, that's backwards. In CANFAIL mode, mallocarray returns NULL > instead of panicing immediately. It's a kludge so the caller doesn't > have to do overflow checking. Then it's a horrible interface. We failed badly with the MPSAFE interface. It was OK at first, but then when everybody uses it, then it because obvious that it was the wrong decision. > > That=E2=80=99s got to be at most a transient thing until all the > > code that it is kludged into with out proper thought is fixed. > > You mean the panic? What fallback behavior would you prefer? If the > caller requested an overflowing allocation, there really isn't > anything sane to do besides immediately panic (again, for M_WAITOK). > Even M_NOWAIT callers may or may not do something sane. > I'd prefer that we return NULL. Let the caller decide the policy, not some stupid thing down in the bowls of malloc because I forgot to include some stupid new flag. M_NOWAIT callers know to check for NULL, or they have a bug. It would be the same for mallocarray: you must check for NULL and unwind if you get that back. There's no need for the CANFAIL flag and it's a horrible API. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ>