Date: Tue, 20 May 2014 21:38:50 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Peter Holm <pho@freebsd.org> Cc: freebsd-gnats-submit@freebsd.org, freebsd-i386@freebsd.org Subject: Re: i386/189955: makecontext(3) argument validation seems broken on i386 and powerpc Message-ID: <20140520205702.E2448@besplex.bde.org> In-Reply-To: <201405191606.s4JG6oq8029605@cgiserv.freebsd.org> References: <201405191606.s4JG6oq8029605@cgiserv.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 19 May 2014, Peter Holm wrote:
>> Description:
> The argc validation seems broken:
Why should it do any validation at all? Its man page documents (much more
clearly than most man pages) that the behaviour for invalid args is
undefined. It can't possibly do any useful validation, since ucp points
to mostly-opaque data. All it can do is check that ucp is not a null pointer
and that a bit of data pointed to it is reasonable, and that argc is
reasonable. It also cannot check that the extra args given by argc
are reasonable.
> $ grep -rw NCARGS /usr/include
> /usr/include/sys/param.h:#define NCARGS ARG_MAX /* max bytes for an exec function */
Bogus. makecontext() isn't an exec function. A random integer would work
almost as well here. An arbitrary limit gratuitously restricts
makecontext(). Negative and small positive values for the limit would
just restrict it enough to break it. It mallocs storage related to
argc. A large argc can cause the size calculations for the malloc() to
fail, but that is just a special case of undefined behaviour from
invalid args. The behaviour is undefined unless argc matches the actual
number of args. argc is just a helper for the variadic args. The limit
on the number of args for makecontext() is equally poorly documented to
the limit on the number of args for a C function. The C standard requires
some minimum limit that is too small, and IIRC it requires the actual
limit to be documented, but documentation about this is hard to find.
In practice, the limit is unreachable except on 16-bit systems (and
when someone uses ulimit(1) to demonstrate brokenness).
> $ egrep "argc < 0.*argc" src/lib/libc/*/gen/makecontext.c
> src/lib/libc/amd64/gen/makecontext.c: else if ((argc < 0) || (argc > 6) || (ucp->uc_stack.ss_sp == NULL) ||
6 is an even more gratuitously restrictive limit.
> src/lib/libc/i386/gen/makecontext.c: else if ((argc < 0) || (argc > NCARGS)) {
> src/lib/libc/ia64/gen/makecontext.c: if (argc < 0 || argc > 8 || ucp == NULL ||
> src/lib/libc/mips/gen/makecontext.c: if (argc < 0 || argc > 6 || ucp == NULL ||
> src/lib/libc/powerpc/gen/makecontext.c: if ((ucp == NULL) || (argc < 0) || (argc > NCARGS)
> src/lib/libc/powerpc64/gen/makecontext.c: if ((ucp == NULL) || (argc < 0) || (argc > NCARGS)
> src/lib/libc/sparc64/gen/makecontext.c: if ((argc < 0) || (argc > 6) ||
The only bugs I see here are:
- the existence of these checks
- the style bugs in them:
- excessive parentheses for amd64, i386, powerpc and sparc64
- other gratuitous differences, like having the ucp == NULL check first
for powerpc and elsewhere for amd64, i386 and sparc64. The complete
file differences are even larger due to different birthplaces and not
much convergence.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140520205702.E2448>