svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/...

[Warner Losh]

On Wed, Jan 24, 2018 at 10:34 AM, Conrad Meyer <cem at freebsd.org> wrote:

> On Wed, Jan 24, 2018 at 7:44 AM, Warner Losh <imp at bsdimp.com> wrote:
> > I agree completely. It doesn't do what you think it is doing, for all the
> > reasons that Bruce outlines. We thought it was a bad idea when it came
> up 2
> > years ago and nothing has really changed.
>
> I disagree.  I'm not sure what you mean by "it doesn't do what you
> think it is doing."  Do you think the manual page is unclear or needs
> more detail?  It seems clear to me, but it also does what I think it
> does.
>

It changes the fundamental 'can't fail' allocations into 'maybe panic'
allocations, which was my big objection.


> Your description of two years ago is inaccurate — you thought it was a
> bad idea, and were the most vocal on the mailing list about it, but
> that viewpoint was not universally shared.  In a pure headcount vote I
> think you were even outvoted, but as the initiative was headed by a
> non-committer, it sputtered out.
>

I don't recall that happening. But regardless, mallocarray, as implemented
today, is useless.


> If Bruce has made some important point or illumination, please
> highlight it.  It's buried in the mostly nonsense wall of text
> boilerplate he usually includes.
>

Let's start with his point about u_long vs size_t causing problems:

void    *malloc(unsigned long size, struct malloc_type *type, int flags)
vs
void    *mallocarray(size_t nmemb, size_t size, struct malloc_type *type,

Since size_t is long long on i386, for example, this can result in
undetected overflows. Such inattention to detail makes me extremely uneasy
about the rest of the code.

mallocarray serves an important function — a last ditch seatbelt
> against overflowing allocations that can trivially replace existing
> naive malloc calls containing multiplication.  Trivial heap corruption
> is replaced with DoS — a strict improvement.  That is all it does.
>

It's an important function, but it's so poorly implement we should try
again. It is not safe nor wise to use it blindly, which was bde's larger
point.

#define MUL_NO_OVERFLOW         (1UL << (sizeof(size_t) * 8 / 2))
static inline bool
WOULD_OVERFLOW(size_t nmemb, size_t size)
{

        return ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
            nmemb > 0 && __SIZE_T_MAX / nmemb < size);
}

So if I pass in 1GB and 10, this will tell me it won't overflow because of
size_t can handle 10GB, but u_long can't. On amd64, it won't, but on i386
it will since long is 32 bits leading to issues in this code:

void *
mallocarray(size_t nmemb, size_t size, struct malloc_type *type, int flags)
{

        if (WOULD_OVERFLOW(nmemb, size))
                panic("mallocarray: %zu * %zu overflowed", nmemb, size);

        return (malloc(size * nmemb, type, flags));
}

since malloc takes u_long, size * nmemb would overflow yielding bad results.

Many places that use mallocarray likely should use WOULD_OVERFLOW instead
to do proper error handling, assuming it is fixed to match the actual
malloc interface. This is especially true in the NO_WAIT cases.

Warner