Date: Wed, 29 Oct 2014 06:26:42 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kib@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r273784 - in head/sys: amd64/ia32 compat/freebsd32 i386/i386 kern net Message-ID: <20141029042007.N2423@besplex.bde.org> In-Reply-To: <201410281528.s9SFSLs2013764@svn.freebsd.org> References: <201410281528.s9SFSLs2013764@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 28 Oct 2014, Konstantin Belousov wrote:
> Log:
> Replace some calls to fuword() by fueword() with proper error checking.
I just noticed some more API design errors. The pointer type for new
APIs should be [qualifed] wordsize_t *, not [qualified] void *. Using
void * reduces type safety for almost no benefits. The casuword()
family already doesn't use void *.
> Modified: head/sys/kern/kern_exec.c
> ==============================================================================
> --- head/sys/kern/kern_exec.c Tue Oct 28 15:22:13 2014 (r273783)
> +++ head/sys/kern/kern_exec.c Tue Oct 28 15:28:20 2014 (r273784)
> @@ -1091,7 +1091,7 @@ int
> exec_copyin_args(struct image_args *args, char *fname,
> enum uio_seg segflg, char **argv, char **envv)
> {
> - char *argp, *envp;
> + u_long argp, envp;
> int error;
> size_t length;
>
Here you made some changes to reduce the type errors allowed by the bad
type safety. Some places use caddr_t for the pointer type. This would
be correct if caddr_t is actually an opaque type, but many uses of it
require it to be precisely char *. Here the char * was used directly.
> @@ -1127,13 +1127,17 @@ exec_copyin_args(struct image_args *args
> /*
> * extract arguments first
> */
> - while ((argp = (caddr_t) (intptr_t) fuword(argv++))) {
> - if (argp == (caddr_t) -1) {
fuword() returns an integer type, and that is often what is wanted. But
here argv is a pointer to a pointer and we want to follow it. We use lots
of type puns to follow this user pointer in kernel code.
The casts here should have been (char *)(uintptr_t).
char * is the best type for argp, unless you change the API massively
so that fu*word*() represents user pointers using a scalar type
(vm_offset_t, or just a properly opaque caddr_t).
> + for (;;) {
> + error = fueword(argv++, &argp);
> + if (error == -1) {
> error = EFAULT;
> goto err_exit;
> }
> - if ((error = copyinstr(argp, args->endp,
> - args->stringspace, &length))) {
> + if (argp == 0)
> + break;
> + error = copyinstr((void *)(uintptr_t)argp, args->endp,
> + args->stringspace, &length);
char * argp was a better match to the API than u_long. Now it is assumed
(for fueword() to work) that long can represent all user pointers, and
there are many more assumptions that type puns between long and u_long
work.
> + if (error != 0) {
> if (error == ENAMETOOLONG)
> error = E2BIG;
> goto err_exit;
This shows that the void * arg type for fu*word*() provides few benefits
in a complicated case -- you still need some casts to defeat type safety.
In simpler cases, I think the void * arg type just gives the negative
benefit of built-in defeat of type safety. The simple use is:
wordsize_t *user_foo_ptr;
wordsize_t kernel_foo;
..
error = fueword(user_foo_ptr, &kernel_foo);
The new API already enforces some type safety for kernel_foo here (in
the old API, you could easily assign to a kernel_foo of the wrong type).
It is not much to ask that user_foo_ptr has a matching type too. For
argv above, this makes it clear that significant type puns are needed
to go from char ** to wordsize_t *. We already punned away a const.
I just noticed some more type errors:
- wordsize_t is long, to be bug for bug compatible with the old API.
This is more bogus than before, since -1 no longer needs to be
returned in wordsize_t. The casuword() family uses the slightly
better type u_long. vm_offset_t would be more correct.
- the above change takes a trip through u_long instead of a trip
through caddr_t and char *. It should use long directly, given
that the API uses long.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141029042007.N2423>