Date: Tue, 2 Feb 2016 17:05:18 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: cem@freebsd.org, Mike Belopuhov <mike@belopuhov.com>, Ryan Stone <rysto32@gmail.com>, "freebsd-arch@freebsd.org" <arch@freebsd.org> Subject: Re: OpenBSD mallocarray Message-ID: <20160202160137.M916@besplex.bde.org> In-Reply-To: <CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ@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> <CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 1 Feb 2016, Warner Losh wrote:
> 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:
> ...
>>> 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.
The whole API is horrible. It is a wrapper that is intended to make
things simpler, but actually makes them more complicated. It is even
worse than calloc(3) since normal use of malloc(9) is to never fail,
while both malloc(3) and calloc(3) always have failure modes.
It is the array size calculation that can overflow, not the allocation.
First I note how using calloc(3) is only slightly more complicated
unless sloppy error handling is acceptable. Good version that does
checks up front like most malloc(9) users already do, but for malloc(3).
=09if (size !=3D 0 && SIZE_MAX / size < num)
=09=09return (EINVAL);
=09if (malloc((size_t)size * num) =3D=3D NULL)
=09=09laboriously_handle_error_usually_worse_than_null_ptr_core();
The error "can't happen", and we can make that even more likely by
changing SIZE_MAX to a small value. We should usually change SIZE_MAX
to a small value anyway to limit denials of service to other caleers.
The overflow check can be hidden in a macro (not in an inline function
without expanding everything to uintmax_t), but using the macro isn't
much easier:
#define=09MALLOC_ARRAY_CHECK(num, size, limit) \
=09((size) =3D=3D 0 || limit / (size) >=3D (num))
=2E..
=09if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX))
=09=09return (EINVAL);
=09if (malloc((size_t)size * num) =3D=3D NULL)
=09=09laboriously_handle_error_usually_worse_than_null_ptr_core();
Sloppy code using calloc():
=09if (calloc(num, size) =3D=3D NULL)
=09=09laboriously_handle_error_usually_worse_than_null_ptr_core();
Equivalent code using calloc():
=09if (calloc(num, size) =3D=3D NULL) {
=09=09/*
=09=09 * calloc() doesn't tell us the precise reason for failure,
=09=09 * and decoding errno wouldn't be much better than the
=09=09 * following,
=09=09 * so if our API is not as bad as that then we must check
=09=09 * for overflow like we should have done up front.
=09=09 */
=09=09if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX))
=09=09=09return (EINVAL);
=09=09laboriously_handle_error_usually_worse_than_null_ptr_core();
=09}
Using malloc(9) with M_NOWAIT is similar except the error handling must be
even more laborious to be correct. It usually isn't correct, but is more
needed and is sometimes better than a null pointer panic. Using M_WAITOK,
we lose the simpler code that is possible by not having error handling in
every caller. Good version:
=09if (!MALLOC_ARRAY_CHECK(num, size, my_limit))
=09=09return (EINVAL);
=09ptr =3D malloc((size_t)size * num, M_FOO, M_WAITOK);
The error handling is obviously neither neither laborious or incorrect.
It is just to handle a DOS from callers.
Worse version: put all the args in a function. Add flags to control
the error handling. I forget the details of mallocarray(). The above
can be done by adding 2 args:
=09ptr =3D mymallocarray(num, size, sizelimit, M_FOO, M_WAITOK);
=09if (ptr =3D=3D NULL)
=09=09return (EINVAL);
This is actually a couple of words less complicated, but also less
clear. It hides the limit check.
Don't forget to expand the types taken by the function to uintmax_t.
Otherwise, overflow may occur when the function is called when the
type of num, size of sizelimit is larger than size_t (most likely
if it is a 64-bit type on a 32-bit arch). The macro handles this
automatically provided size_max <=3D SIZE_MAX.
Bruce
From owner-freebsd-arch@freebsd.org Wed Feb 3 19:39:29 2016
Return-Path: <owner-freebsd-arch@freebsd.org>
Delivered-To: freebsd-arch@mailman.ysv.freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org
[IPv6:2001:1900:2254:206a::19:1])
by mailman.ysv.freebsd.org (Postfix) with ESMTP id 79613A9AE53;
Wed, 3 Feb 2016 19:39:29 +0000 (UTC) (envelope-from jhb@freebsd.org)
Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1])
(using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id 5351B1F3E;
Wed, 3 Feb 2016 19:39:29 +0000 (UTC) (envelope-from jhb@freebsd.org)
Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net
[73.231.226.104])
by bigwig.baldwin.cx (Postfix) with ESMTPSA id DB9B7B917;
Wed, 3 Feb 2016 14:39:27 -0500 (EST)
From: John Baldwin <jhb@freebsd.org>
To: freebsd-arch@freebsd.org
Cc: Warner Losh <imp@bsdimp.com>, Brooks Davis <brooks@freebsd.org>,
Mike Belopuhov <mike@belopuhov.com>, Ryan Stone <rysto32@gmail.com>,
"freebsd-arch@freebsd.org" <arch@freebsd.org>
Subject: Re: OpenBSD mallocarray
Date: Wed, 03 Feb 2016 11:39:11 -0800
Message-ID: <1955470.jNCaThvui8@ralph.baldwin.cx>
User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; )
In-Reply-To: <CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA@mail.gmail.com>
References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com>
<20160201224854.GB1747@spindle.one-eyed-alien.net>
<CANCZdfqDDE5jigsDUSpLPjoqGNPCqn4kipX-eqYBBYakuTiBWA@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7
(bigwig.baldwin.cx); Wed, 03 Feb 2016 14:39:28 -0500 (EST)
X-BeenThere: freebsd-arch@freebsd.org
X-Mailman-Version: 2.1.20
Precedence: list
List-Id: Discussion related to FreeBSD architecture <freebsd-arch.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/freebsd-arch>,
<mailto:freebsd-arch-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-arch/>;
List-Post: <mailto:freebsd-arch@freebsd.org>
List-Help: <mailto:freebsd-arch-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/freebsd-arch>,
<mailto:freebsd-arch-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 03 Feb 2016 19:39:29 -0000
On Monday, February 01, 2016 04:01:14 PM Warner Losh wrote:
> On Mon, Feb 1, 2016 at 3:48 PM, Brooks Davis <brooks@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@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@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.
>
> I guess this comes down to 'why is it an unreasonable burden to
> test the return value in code that's converted?' We're already changing
> the code.
>
> If you absolutely must have a flag, I'd prefer M_CANPANIC or something
> that is also easy to add for the 'mindless' case that we can easily
> grep for so we know when we're removed all the stupid 'mindless'
> cases from the tree.
Having M_WAITOK-anything return NULL will be a POLA violation. It doesn't
return NULL for anything else. I think having a separate M_CANFAIL flag
is also rather pointless. If we want to have this, I think it should
work similar to malloc(). M_WAITOK panics if you do stupid things
(malloc(9) does this for sufficiently large overflow when it exhausts kmem
contrary to Warner's earlier claim), M_NOWAIT returns NULL.
In general I think I most prefer Bruce's approach of having a separate macro
to check for overflow explicitly so it can be handled as a separate error.
In particular, if mallocarry(..., M_NOWAIT) fails, is it because of memory
shortage (in which case retrying in the future might be sensible) or is it
due to overflow (in which case it will fail no matter how many times you
retry)? You'd then have to have the macro anyway to differentiate and handle
this case.
Warner also seems to be assuming that we should do check for overflow
explicitly for any user-supplied values before calling malloc() or
malloc()-like things. This means N hand-rolled (and possibly buggy) checks,
or a shared macro to do the check. I think this is another argument in favor
of Bruce's approach. :)
If you go that route, then mallocarray() is really just an assertion
checker. It should only fail because a programmer ommitted an explicit
overflow check for a user-supplied value or screwed up an in-kernel
value. In that case I think panic'ing sooner when the overflow is obvious
is more useful for debugging the error than a NULL pointer deference
some time later (or requests that get retried forever and go possibly
unnoticed).
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160202160137.M916>