svn commit: r273784 - in head/sys: amd64/ia32 compat/freebsd32 i386/i386 kern net

[Bruce Evans]

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