Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 8 Jan 2018 10:02:45 -0700
From:      Warner Losh <imp@bsdimp.com>
To:        Mark Johnston <markj@freebsd.org>
Cc:        Ian Lepore <ian@freebsd.org>, Pedro Giffuni <pfg@freebsd.org>, Ed Schouten <ed@nuxi.nl>,  Andrew Turner <andrew@fubar.geek.nz>, Ed Schouten <ed@freebsd.org>,  src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r327684 - in head/sys/compat: cloudabi32 cloudabi64
Message-ID:  <CANCZdfozhyN0nq3wWR1UR2ZtT7BRKemenXJGZUT5yZ9AaPdqGA@mail.gmail.com>
In-Reply-To: <20180108162938.GD2412@raichu>
References:  <201801072238.w07McjLP099234@repo.freebsd.org> <8D8CA434-2A87-44D9-AC27-5166802FBBC2@fubar.geek.nz> <CABh_MKm4HW2nJ=402oiELgWDo=Q7h15kjOU1p6F2BPOchjZCiw@mail.gmail.com> <0a6ad324-46f2-9270-5abd-dbc3e734cc8b@FreeBSD.org> <CANCZdfq6DZ7_VaQALqrpqydsjJcyZOyV9uWEcTNQWJ_sR2T-cA@mail.gmail.com> <1515428308.44630.2.camel@freebsd.org> <20180108162938.GD2412@raichu>

next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jan 8, 2018 at 9:29 AM, Mark Johnston <markj@freebsd.org> wrote:

> On Mon, Jan 08, 2018 at 09:18:28AM -0700, Ian Lepore wrote:
> > On Mon, 2018-01-08 at 09:13 -0700, Warner Losh wrote:
> > > On Jan 8, 2018 8:37 AM, "Pedro Giffuni" <pfg@freebsd.org> wrote:
> > > On 01/08/18 10:13, Ed Schouten wrote:
> > >
> > > >
> > > > Hi Andrew,
> > > >
> > > > 2018-01-08 8:37 GMT+01:00 Andrew Turner <andrew@fubar.geek.nz>:
> > > >
> > > > >
> > > > > Won=E2=80=99t this lead to a NULL pointer dereference on overflow=
?
> mallocarray
> > > > > can return NULL even with M_WAITOK.
> > > > >
> > > > Yes, it will, but an overflow shouldn't happen in the first place.
> > > > ri_data_len is compared with UIO_MAXIOV a few lines above. Even if =
an
> > > > overflow would happen, this would cause a kernel panic due to a NUL=
L
> > > > pointer dereference later on, which is likely easier to debug than
> > > > some piece of code that overruns a buffer.
> > > >
> > > > In this case, mallocarray() is preferred, because it makes it more
> > > > obvious that we're allocating a buffer that is accessed as an array=
,
> > > > as opposed to single structure.
> > > >
> > > > OK...
> > > The behavior of mallocarray() somewhat inconsistent with malloc(9),
> > > realloc(9) and reallocf(9) but this is clearly documented., so we jus=
t
> > > assume the developer knows what he/she is doing :).
> > >
> > >
> > > This is one reason it didn't go in before... the error semantics
> suck... we
> > > re are a poor match for existing code.
> > >
> > > Warner
> >
> > Yeah, having a bunch of functions with malloc in the name, all taking
> > the same M_WAITOK flag, but that flag has different implications for
> > calling code in regards to just one of the malloc functions...
>
> contigmalloc(M_WAITOK) isn't guaranteed to succeed either. In that case,
> M_WAITOK just means "try harder to defragment physical memory in the
> request space before giving up."
>
> > that's just a recipe for creating bugs.  It makes this whole function a
> bad
> > idea.
>
> A NULL return value from mallocarray() indicates a bug in the caller. I
> don't see why it isn't preferable to crash quickly and loudly in that
> case.
>

When this came up before, people wanted a check_mallocarray(a, b) so they
could centralize all the integer overflow knowledge in one place...

Seems like we're creating ABIs that are more error prone than the problem
we're trying to catch...

Warner



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfozhyN0nq3wWR1UR2ZtT7BRKemenXJGZUT5yZ9AaPdqGA>