From owner-svn-src-all@FreeBSD.ORG Sat Jan 3 09:01:10 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E877EBA5; Sat, 3 Jan 2015 09:01:09 +0000 (UTC) Received: from dchagin.static.corbina.net (dchagin.static.corbina.ru [78.107.232.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "dchagin.static.corbina.net", Issuer "dchagin.static.corbina.net" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 82986118B; Sat, 3 Jan 2015 09:01:08 +0000 (UTC) Received: from dchagin.static.corbina.net (localhost [127.0.0.1]) by dchagin.static.corbina.net (8.14.9/8.14.9) with ESMTP id t03914BT028995 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 3 Jan 2015 12:01:05 +0300 (MSK) (envelope-from dchagin@dchagin.static.corbina.net) Received: (from dchagin@localhost) by dchagin.static.corbina.net (8.14.9/8.14.9/Submit) id t03914Uj028994; Sat, 3 Jan 2015 12:01:04 +0300 (MSK) (envelope-from dchagin) Date: Sat, 3 Jan 2015 12:01:04 +0300 From: Chagin Dmitry To: Bruce Evans Subject: Re: svn commit: r276564 - head/sys/compat/linux Message-ID: <20150103090104.GA28933@dchagin.static.corbina.net> References: <201501021929.t02JTXd7093292@svn.freebsd.org> <20150103175627.K979@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150103175627.K979@besplex.bde.org> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 03 Jan 2015 09:01:10 -0000 On Sat, Jan 03, 2015 at 06:39:48PM +1100, Bruce Evans wrote: > On Fri, 2 Jan 2015, Dmitry Chagin wrote: > > > Log: > > Cast *path to silence clang -Wpointer-sign warning. > > Why not fix the bug instead of breaking the warning? (You usually do > this better.) > > > Modified: head/sys/compat/linux/linux_getcwd.c > > ============================================================================== > > --- head/sys/compat/linux/linux_getcwd.c Fri Jan 2 19:06:27 2015 (r276563) > > +++ head/sys/compat/linux/linux_getcwd.c Fri Jan 2 19:29:32 2015 (r276564) > > @@ -430,7 +430,7 @@ linux_getcwd(struct thread *td, struct l > > > > path = (char *)malloc(len, M_TEMP, M_WAITOK); > > > > - error = kern___getcwd(td, path, UIO_SYSSPACE, len); > > + error = kern___getcwd(td, (u_char *)path, UIO_SYSSPACE, len); > > The bug is that __getcwd(2undoc) has a bogus API that is mismatched with > all callers and also with its implementation. > > libc/gen/getcwd.c misdeclares __getcwd() as taking a plain char * path, but > this type mismatch is not detected since libc/gen/getcwd.c doesn't include > the header where the syscall is declared (sys/syscallsubr.h). This type > pun prevents clang detecting the same problem as above (pathnames are > always char * except when bogusly passed to *__getcwd()). > > __kern_getcwd() internally doesn't actually use or support u_char * paths, > except to copy them to a normal char * path. The copying is done using > bcopy(), so its behaviour is not undefined like it is with the type pun, > but it is still logically wrong and would be physically wrong if signed > chars were exotic. The behaviour is only clearly correct if the pointer: > 1) starts as plain char * > 2) actually points to valid plain chars > 3) is converted without type puns before use, either back to the original > plain char * pointer for direct use, or to void * for use in bcopy() > and then plain char * is used to access the copy. > The above bogus cast gives this. This depends only __kern_getcwd() not > actually supporting u_char * but doing step (3). > > I think the bogus cast from 'signed foo_t *' to 'unsigned foo_t *' gives > undefined behaviour in general, but for foo_t = char it only gives > implementation-defined behaviour (different interpretations of the sign > bit and +-0), at least in POSIX where chars are 8 bits so there is no > space for extra bits. The only case of even theoretical interest is > +-0 on 1's complement systems with plain char = signed char . Then +-0 > cannot be distinguished as chars. FreeBSD just doesn't support such > systems. At least to the extent of handling the file name "\xff" on > a removable disk generated on a 2's complement system. > > The cast of malloc() is also bogus. > Thanks, Bruce! So, let's change kern___getcwd() definition: diff --git a/sys/compat/freebsd32/syscalls.master b/sys/compat/freebsd32/syscalls.master index 5d445a5..ce655bc 100644 --- a/sys/compat/freebsd32/syscalls.master +++ b/sys/compat/freebsd32/syscalls.master @@ -583,7 +583,7 @@ 323 AUE_NULL OBSOL thr_wakeup 324 AUE_MLOCKALL NOPROTO { int mlockall(int how); } 325 AUE_MUNLOCKALL NOPROTO { int munlockall(void); } -326 AUE_GETCWD NOPROTO { int __getcwd(u_char *buf, u_int buflen); } +326 AUE_GETCWD NOPROTO { int __getcwd(char *buf, u_int buflen); } 327 AUE_NULL NOPROTO { int sched_setparam (pid_t pid, \ const struct sched_param *param); } diff --git a/sys/compat/linux/linux_getcwd.c b/sys/compat/linux/linux_getcwd.c index 1278721..448c404 100644 --- a/sys/compat/linux/linux_getcwd.c +++ b/sys/compat/linux/linux_getcwd.c @@ -186,7 +186,7 @@ linux_getcwd_scandir(lvpp, uvpp, bpp, bufp, td) dirbuflen = DIRBLKSIZ; if (dirbuflen < va.va_blocksize) dirbuflen = va.va_blocksize; - dirbuf = (char *)malloc(dirbuflen, M_LINUX, M_WAITOK); + dirbuf = malloc(dirbuflen, M_LINUX, M_WAITOK); #if 0 unionread: @@ -413,7 +413,7 @@ out: int linux_getcwd(struct thread *td, struct linux_getcwd_args *args) { - caddr_t bp, bend, path; + char *bp, *bend, *path; int error, len, lenused; #ifdef DEBUG @@ -428,9 +428,9 @@ linux_getcwd(struct thread *td, struct linux_getcwd_args *args) else if (len < 2) return ERANGE; - path = (char *)malloc(len, M_LINUX, M_WAITOK); + path = malloc(len, M_LINUX, M_WAITOK); - error = kern___getcwd(td, (u_char *)path, UIO_SYSSPACE, len); + error = kern___getcwd(td, path, UIO_SYSSPACE, len); if (!error) { lenused = strlen(path) + 1; if (lenused <= args->bufsize) { diff --git a/sys/kern/syscalls.master b/sys/kern/syscalls.master index e0f8b61..089896b 100644 --- a/sys/kern/syscalls.master +++ b/sys/kern/syscalls.master @@ -571,7 +571,7 @@ 323 AUE_NULL OBSOL thr_wakeup 324 AUE_MLOCKALL STD { int mlockall(int how); } 325 AUE_MUNLOCKALL STD { int munlockall(void); } -326 AUE_GETCWD STD { int __getcwd(u_char *buf, u_int buflen); } +326 AUE_GETCWD STD { int __getcwd(char *buf, u_int buflen); } 327 AUE_NULL STD { int sched_setparam (pid_t pid, \ const struct sched_param *param); } diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c index 55e3217..398d21c 100644 --- a/sys/kern/vfs_cache.c +++ b/sys/kern/vfs_cache.c @@ -1043,14 +1043,6 @@ vfs_cache_lookup(ap) return (error); } - -#ifndef _SYS_SYSPROTO_H_ -struct __getcwd_args { - u_char *buf; - u_int buflen; -}; -#endif - /* * XXX All of these sysctls would probably be more productive dead. */ @@ -1060,16 +1052,14 @@ SYSCTL_INT(_debug, OID_AUTO, disablecwd, CTLFLAG_RW, &disablecwd, 0, /* Implementation of the getcwd syscall. */ int -sys___getcwd(td, uap) - struct thread *td; - struct __getcwd_args *uap; +sys___getcwd(struct thread *td, struct __getcwd_args *uap) { return (kern___getcwd(td, uap->buf, UIO_USERSPACE, uap->buflen)); } int -kern___getcwd(struct thread *td, u_char *buf, enum uio_seg bufseg, u_int buflen) +kern___getcwd(struct thread *td, char *buf, enum uio_seg bufseg, u_int buflen) { char *bp, *tmpbuf; struct filedesc *fdp; diff --git a/sys/sys/syscallsubr.h b/sys/sys/syscallsubr.h index 8f074cc..75138ba 100644 --- a/sys/sys/syscallsubr.h +++ b/sys/sys/syscallsubr.h @@ -60,7 +60,7 @@ struct __wrusage; int do_execve(struct thread *td, struct image_args *args, struct mac *mac_p); -int kern___getcwd(struct thread *td, u_char *buf, enum uio_seg bufseg, +int kern___getcwd(struct thread *td, char *buf, enum uio_seg bufseg, u_int buflen); int kern_accept(struct thread *td, int s, struct sockaddr **name, socklen_t *namelen, struct file **fp); -- Have fun! chd