OpenBSD mallocarray

[Bruce Evans]

On Mon, 1 Feb 2016, Warner Losh wrote:

> On Mon, Feb 1, 2016 at 3:19 PM, Conrad Meyer <cem at freebsd.org> wrote:
>
>> On Mon, Feb 1, 2016 at 1:12 PM, Warner Losh <imp at bsdimp.com> wrote:
> ...
>>> 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.
>>
>> 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).

 	if (size != 0 && SIZE_MAX / size < num)
 		return (EINVAL);
 	if (malloc((size_t)size * num) == NULL)
 		laboriously_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	MALLOC_ARRAY_CHECK(num, size, limit) \
 	((size) == 0 || limit / (size) >= (num))
...
 	if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX))
 		return (EINVAL);
 	if (malloc((size_t)size * num) == NULL)
 		laboriously_handle_error_usually_worse_than_null_ptr_core();

Sloppy code using calloc():

 	if (calloc(num, size) == NULL)
 		laboriously_handle_error_usually_worse_than_null_ptr_core();

Equivalent code using calloc():

 	if (calloc(num, size) == NULL) {
 		/*
 		 * calloc() doesn't tell us the precise reason for failure,
 		 * and decoding errno wouldn't be much better than the
 		 * following,
 		 * so if our API is not as bad as that then we must check
 		 * for overflow like we should have done up front.
 		 */
 		if (!MALLOC_ARRAY_CHECK(num, size, SIZE_MAX))
 			return (EINVAL);
 		laboriously_handle_error_usually_worse_than_null_ptr_core();
 	}

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:

 	if (!MALLOC_ARRAY_CHECK(num, size, my_limit))
 		return (EINVAL);
 	ptr = 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:

 	ptr = mymallocarray(num, size, sizelimit, M_FOO, M_WAITOK);
 	if (ptr == NULL)
 		return (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 <= SIZE_MAX.

Bruce