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
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
