Date: Sat, 3 Jan 2015 12:01:04 +0300 From: Chagin Dmitry <dchagin@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r276564 - head/sys/compat/linux Message-ID: <20150103090104.GA28933@dchagin.static.corbina.net> In-Reply-To: <20150103175627.K979@besplex.bde.org> References: <201501021929.t02JTXd7093292@svn.freebsd.org> <20150103175627.K979@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150103090104.GA28933>