From owner-svn-src-head@FreeBSD.ORG Tue Oct 28 19:26:53 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 63342872; Tue, 28 Oct 2014 19:26:53 +0000 (UTC) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id D58BDAE2; Tue, 28 Oct 2014 19:26:52 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 355F4D47447; Wed, 29 Oct 2014 06:26:44 +1100 (AEDT) Date: Wed, 29 Oct 2014 06:26:42 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov Subject: Re: svn commit: r273784 - in head/sys: amd64/ia32 compat/freebsd32 i386/i386 kern net In-Reply-To: <201410281528.s9SFSLs2013764@svn.freebsd.org> Message-ID: <20141029042007.N2423@besplex.bde.org> References: <201410281528.s9SFSLs2013764@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=BdjhjNd2 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=3o5ZHGEtQ1bD-Fw3Y7AA:9 a=3QTGIgrYzqncwDpa:21 a=QpfHI327jUG1pkl_:21 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Oct 2014 19:26:53 -0000 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