Large Capsicum patch for review.

Christoph Mallon christoph.mallon at gmx.de
Sun Feb 24 15:07:22 UTC 2013


On 24.02.2013 07:05, Christoph Mallon wrote:
> I started reading the patch and found some minor glitches, e.g. mode in cap_sandboxed() should be u_int, not int.
> I will report more later.

I placed several patches with suggested changes at http://tron.homeunix.org/zeug/FreeBSD/capsicum/ (I also attached them).
Please have a look at them.
I also have some comments:
- A bitmask for cap_rights_limit() seems limited.
  There are only six bits left unused.
  Maybe a more general mechanism should be used.
- I think that CAP_SEND, CAP_RECV, CAP_FCHMODAT, CAP_FCHOWNAT, CAP_FSTATAT and CAP_FUTIMESAT are pointless aliases, which provide no benefit, but rather might cause confusion, whether there is a difference between e.g. CAP_WRITE and CAP_SEND.
- Why did you choose INT_MAX to represent "all capabilities"?
  The data type used is ssize_t, so SSIZE_MAX seems more logical.
  Further, there are several places where size_t and ssize_t come into contact, which need careful tests and casts to get right.
  Maybe it is better to always use size_t and represent "all capabilities" with SIZE_T_MAX or (size_t)-1 (which is guaranteed by C to be the maximal value).
- Is it correct, that fdp seems to be not locked in fp_getfvp()?
  Otherweise, fget_locked() could be used instead of the manual check.
- I could not verify many changes, e.g. changed requested permissions, because this is just a big diff with no information about the intention of the changes.
  A series of smaller diffs with commit logs to state their intent would be really useful for reviewing.

	Christoph
-------------- next part --------------
From 7f18186c451c235bcc3dbf2e25f65613e1ce7f16 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:01:00 +0100
Subject: [PATCH 02/24] Use the correct type for the parameter of
 cap_getmode().

---
 lib/libc/gen/cap_sandboxed.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/libc/gen/cap_sandboxed.c b/lib/libc/gen/cap_sandboxed.c
index 95e5b23..5743f2a 100644
--- a/lib/libc/gen/cap_sandboxed.c
+++ b/lib/libc/gen/cap_sandboxed.c
@@ -39,7 +39,7 @@ __FBSDID("$FreeBSD$");
 bool
 cap_sandboxed(void)
 {
-	int mode;
+	u_int mode;
 
 	if (cap_getmode(&mode) == -1) {
 		assert(errno == ENOSYS);
-- 
1.8.1.3

-------------- next part --------------
From 7ad09a0cf0ceb9828171133e32f7148ffde62d97 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:03:03 +0100
Subject: [PATCH 03/24] Use cheaper != 0 tests.

---
 lib/libc/gen/cap_sandboxed.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/libc/gen/cap_sandboxed.c b/lib/libc/gen/cap_sandboxed.c
index 5743f2a..f09b590 100644
--- a/lib/libc/gen/cap_sandboxed.c
+++ b/lib/libc/gen/cap_sandboxed.c
@@ -41,10 +41,10 @@ cap_sandboxed(void)
 {
 	u_int mode;
 
-	if (cap_getmode(&mode) == -1) {
+	if (cap_getmode(&mode) != 0) {
 		assert(errno == ENOSYS);
 		return (false);
 	}
 	assert(mode == 0 || mode == 1);
-	return (mode == 1);
+	return (mode != 0);
 }
-- 
1.8.1.3

-------------- next part --------------
From ae6ac8b82374e83dcc2bda7dd6e17da5d65d9eae Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:03:24 +0100
Subject: [PATCH 04/24] Add cap_sandboxed to Symbol.map.

---
 lib/libc/sys/Symbol.map | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/libc/sys/Symbol.map b/lib/libc/sys/Symbol.map
index 376ace0..7738e46 100644
--- a/lib/libc/sys/Symbol.map
+++ b/lib/libc/sys/Symbol.map
@@ -384,6 +384,7 @@ FBSD_1.3 {
 	cap_ioctls_limit;
 	cap_rights_get;
 	cap_rights_limit;
+	cap_sandboxed;
 	clock_getcpuclockid2;
 	ffclock_getcounter;
 	ffclock_getestimate;
-- 
1.8.1.3

-------------- next part --------------
From 4822faec8ea5c53c673740eeec0cb604960af37b Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:03:59 +0100
Subject: [PATCH 05/24] Use standard inline instead of the GNUism __inline.

---
 sys/kern/sys_capability.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index 941ea49..0f4ef12 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -144,7 +144,7 @@ sys_cap_getmode(struct thread *td, struct cap_getmode_args *uap)
 
 FEATURE(security_capabilities, "Capsicum Capabilities");
 
-static __inline int
+static inline int
 _cap_check(cap_rights_t have, cap_rights_t need, enum ktr_cap_fail_type type)
 {
 
-- 
1.8.1.3

-------------- next part --------------
From 1d1defdf7dd117afa56330c958daeac5d52be05f Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:04:41 +0100
Subject: [PATCH 06/24] Avoid polluting the global namespace with stdbool.h.

---
 sys/sys/capability.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sys/sys/capability.h b/sys/sys/capability.h
index df95ae4..9eed4d1 100644
--- a/sys/sys/capability.h
+++ b/sys/sys/capability.h
@@ -251,7 +251,6 @@ int	cap_fcntl_check(struct filedesc *fdp, int fd, int cmd);
 #else /* !_KERNEL */
 
 __BEGIN_DECLS
-#include <stdbool.h>
 
 /*
  * cap_enter(): Cause the process to enter capability mode, which will
@@ -270,7 +269,7 @@ int	cap_enter(void);
  * Are we sandboxed (in capability mode)?
  * This is libc wrapper around cap_getmode(2) system call.
  */
-bool	cap_sandboxed(void);
+_Bool	cap_sandboxed(void);
 
 /*
  * cap_getmode(): Are we in capability mode?
-- 
1.8.1.3

-------------- next part --------------
From 5107ab9aec6766c9ce5285939637c5739b2aed0c Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:05:19 +0100
Subject: [PATCH 07/24] Use ${CC} instead of hardcoding gcc.

---
 tools/regression/capsicum/syscalls/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/regression/capsicum/syscalls/Makefile b/tools/regression/capsicum/syscalls/Makefile
index cb1b07b..5d34226 100644
--- a/tools/regression/capsicum/syscalls/Makefile
+++ b/tools/regression/capsicum/syscalls/Makefile
@@ -14,7 +14,7 @@ all:	${SYSCALLS} ${SYSCALLS:=.t}
 .for SYSCALL in ${SYSCALLS}
 
 ${SYSCALL}:	${SYSCALL}.c misc.c
-	gcc ${CFLAGS} ${@}.c misc.c -o $@
+	${CC} ${CFLAGS} ${@}.c misc.c -o $@
 
 ${SYSCALL}.t:	${SYSCALL}
 	@printf "#!/bin/sh\n\n%s/%s\n" ${.CURDIR} ${@:.t=} > $@
-- 
1.8.1.3

-------------- next part --------------
From d2fe7a6f9d8e08943600b1f5d6a2818c6c4aa794 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:31:37 +0100
Subject: [PATCH 08/24] Use fget_locked() instead of duplicating it.

---
 sys/kern/uipc_usrreq.c | 3 +--
 sys/netsmb/smb_dev.c   | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index 15a1c8a..529d2eb 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -1858,8 +1858,7 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 			FILEDESC_SLOCK(fdesc);
 			for (i = 0; i < oldfds; i++) {
 				fd = *fdp++;
-				if (fd < 0 || fd >= fdesc->fd_nfiles ||
-				    fdesc->fd_ofiles[fd].fde_file == NULL) {
+				if (fget_locked(fdp, fd) == NULL) {
 					FILEDESC_SUNLOCK(fdesc);
 					error = EBADF;
 					goto out;
diff --git a/sys/netsmb/smb_dev.c b/sys/netsmb/smb_dev.c
index 0cee325..a09d74d 100644
--- a/sys/netsmb/smb_dev.c
+++ b/sys/netsmb/smb_dev.c
@@ -399,9 +399,7 @@ nsmb_getfp(struct filedesc* fdp, int fd, int flag)
 	struct file* fp;
 
 	FILEDESC_SLOCK(fdp);
-	if (fd < 0 || fd >= fdp->fd_nfiles ||
-	    (fp = fdp->fd_ofiles[fd].fde_file) == NULL ||
-	    (fp->f_flag & flag) == 0) {
+	if ((fp = fget_locked(fdp, fd)) == NULL || (fp->f_flag & flag) == 0) {
 		FILEDESC_SUNLOCK(fdp);
 		return (NULL);
 	}
-- 
1.8.1.3

-------------- next part --------------
From cc2de42e944828943c17fcba2a821becc634b9a4 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:33:49 +0100
Subject: [PATCH 09/24] Remove duplicate checks.

fget_locked() just below does the same checks.
---
 sys/kern/kern_descrip.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index bed5d8f..473ab40 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2459,11 +2459,6 @@ fgetvp_rights(struct thread *td, int fd, cap_rights_t need,
 	if (td == NULL || (fdp = td->td_proc->p_fd) == NULL)
 		return (EBADF);
 
-	FILEDESC_LOCK_ASSERT(fdp);
-
-	if (fd < 0 || fd >= fdp->fd_nfiles)
-		return (EBADF);
-
 	fp = fget_locked(fdp, fd);
 	if (fp == NULL || fp->f_ops == &badfileops)
 		return (EBADF);
-- 
1.8.1.3

-------------- next part --------------
From 2b9add7396a9485add6a4246071cf041a50161d1 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:34:43 +0100
Subject: [PATCH 10/24] Make fp_getfvp() static.

---
 sys/fs/nfs/nfsdport.h           | 2 --
 sys/fs/nfsserver/nfs_nfsdport.c | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/sys/fs/nfs/nfsdport.h b/sys/fs/nfs/nfsdport.h
index 529ada2..a09a6dd 100644
--- a/sys/fs/nfs/nfsdport.h
+++ b/sys/fs/nfs/nfsdport.h
@@ -94,8 +94,6 @@ struct nfsexstuff {
 #define	NFSFPCRED(f)	((f)->f_cred)
 #define	NFSFPFLAG(f)	((f)->f_flag)
 
-int fp_getfvp(NFSPROC_T *, int, struct file **, struct vnode **);
-
 #define	NFSNAMEICNDSET(n, c, o, f)	do {				\
 	(n)->cn_cred = (c);						\
 	(n)->cn_nameiop = (o);						\
diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c
index cd7814c..880f965 100644
--- a/sys/fs/nfsserver/nfs_nfsdport.c
+++ b/sys/fs/nfsserver/nfs_nfsdport.c
@@ -2767,7 +2767,7 @@ out:
 /*
  * glue for fp.
  */
-int
+static int
 fp_getfvp(struct thread *p, int fd, struct file **fpp, struct vnode **vpp)
 {
 	struct filedesc *fdp;
-- 
1.8.1.3

-------------- next part --------------
From 4645cc6db54f29c1f5ec32c47794fd135f99cf60 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:37:22 +0100
Subject: [PATCH 11/24] Use simple assignment instead of bcopy() to copy
 structs.

---
 sys/kern/kern_descrip.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 473ab40..94bb74a 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -839,8 +839,7 @@ do_dup(struct thread *td, int flags, int old, int new,
 	/*
 	 * Duplicate the source descriptor.
 	 */
-	bcopy(&fdp->fd_ofiles[old], &fdp->fd_ofiles[new],
-	    sizeof(fdp->fd_ofiles[new]));
+	fdp->fd_ofiles[new] = fdp->fd_ofiles[old];
 	filecaps_copy(&fdp->fd_ofiles[old].fde_caps,
 	    &fdp->fd_ofiles[new].fde_caps);
 	if ((flags & DUP_CLOEXEC) != 0) {
@@ -1374,7 +1373,7 @@ filecaps_copy(const struct filecaps *src, struct filecaps *dst)
 {
 	size_t size;
 
-	bcopy(src, dst, sizeof(*dst));
+	*dst = *src;
 	if (src->fc_ioctls != NULL) {
 		KASSERT(src->fc_nioctls > 0,
 		    ("fc_ioctls != NULL, but fc_nioctls=%hd", src->fc_nioctls));
@@ -1392,7 +1391,7 @@ static void
 filecaps_move(struct filecaps *src, struct filecaps *dst)
 {
 
-	bcopy(src, dst, sizeof(*dst));
+	*dst = *src;
 	bzero(src, sizeof(*src));
 }
 
@@ -1835,7 +1834,7 @@ fdcopy(struct filedesc *fdp)
 		    (ofde->fde_file->f_ops->fo_flags & DFLAG_PASSABLE) &&
 		    ofde->fde_file->f_ops != &badfileops) {
 			nfde = &newfdp->fd_ofiles[i];
-			bcopy(ofde, nfde, sizeof(*nfde));
+			*nfde = *ofde;
 			filecaps_copy(&ofde->fde_caps, &nfde->fde_caps);
 			fhold(nfde->fde_file);
 			newfdp->fd_lastfile = i;
@@ -2681,8 +2680,7 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
 			return (EACCES);
 		}
 		fhold(fp);
-		bcopy(&fdp->fd_ofiles[dfd], &fdp->fd_ofiles[indx],
-		    sizeof(fdp->fd_ofiles[indx]));
+		fdp->fd_ofiles[indx] = fdp->fd_ofiles[dfd];
 		filecaps_copy(&fdp->fd_ofiles[dfd].fde_caps,
 		    &fdp->fd_ofiles[indx].fde_caps);
 		break;
@@ -2690,8 +2688,7 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
 		/*
 		 * Steal away the file pointer from dfd and stuff it into indx.
 		 */
-		bcopy(&fdp->fd_ofiles[dfd], &fdp->fd_ofiles[indx],
-		    sizeof(fdp->fd_ofiles[indx]));
+		fdp->fd_ofiles[indx] = fdp->fd_ofiles[dfd];
 		bzero(&fdp->fd_ofiles[dfd], sizeof(fdp->fd_ofiles[dfd]));
 		fdunused(fdp, dfd);
 		break;
-- 
1.8.1.3

-------------- next part --------------
From fc8878d0a43ea3f9d77b1fd51f90f4b4be188fb2 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:40:32 +0100
Subject: [PATCH 12/24] Remove redundant null pointer tests before free().

---
 sys/kern/kern_descrip.c   |  3 +--
 sys/kern/sys_capability.c | 15 +++++----------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 94bb74a..53b24b1 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1415,8 +1415,7 @@ void
 filecaps_free(struct filecaps *fcaps)
 {
 
-	if (fcaps->fc_ioctls != NULL)
-		free(fcaps->fc_ioctls, M_TEMP);
+	free(fcaps->fc_ioctls, M_TEMP);
 	bzero(fcaps, sizeof(*fcaps));
 }
 
diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index 0f4ef12..ecb3a6b 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -228,10 +228,8 @@ sys_cap_rights_limit(struct thread *td, struct cap_rights_limit_args *uap)
 	if (error == 0) {
 		fdp->fd_ofiles[fd].fde_rights = rights;
 		if ((rights & CAP_IOCTL) == 0) {
-			if (fdp->fd_ofiles[fd].fde_ioctls != NULL) {
-				free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP);
-				fdp->fd_ofiles[fd].fde_ioctls = NULL;
-			}
+			free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP);
+			fdp->fd_ofiles[fd].fde_ioctls = NULL;
 			fdp->fd_ofiles[fd].fde_nioctls = 0;
 		}
 		if ((rights & CAP_FCNTL) == 0 &&
@@ -376,8 +374,7 @@ sys_cap_ioctls_limit(struct thread *td, struct cap_ioctls_limit_args *uap)
 
 	FILEDESC_XUNLOCK(fdp);
 
-	if (ocmds != NULL)
-		free(ocmds, M_TEMP);
+	free(ocmds, M_TEMP);
 
 	return (0);
 }
@@ -556,10 +553,8 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
 	 */
 	fdp->fd_ofiles[newfd].fde_rights = rights;
 	if ((rights & CAP_IOCTL) == 0) {
-		if (fdp->fd_ofiles[newfd].fde_ioctls != NULL) {
-			free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP);
-			fdp->fd_ofiles[newfd].fde_ioctls = NULL;
-		}
+		free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP);
+		fdp->fd_ofiles[newfd].fde_ioctls = NULL;
 		fdp->fd_ofiles[newfd].fde_nioctls = 0;
 	}
 	if ((rights & CAP_FCNTL) == 0 && fdp->fd_ofiles[newfd].fde_fcntls != 0)
-- 
1.8.1.3

-------------- next part --------------
From 41fc1f39a18331f57e6a03b54f4b78cbbd123cd5 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:41:38 +0100
Subject: [PATCH 13/24] Remove redundant test.

if needrights is 0, the inner tests will succeed.
---
 sys/kern/kern_descrip.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 53b24b1..d1465a0 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2270,15 +2270,13 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t needrights,
 			return (EBADF);
 #ifdef CAPABILITIES
 		haverights = cap_rights(fdp, fd);
-		if (needrights != 0) {
-			error = cap_check(haverights, needrights);
+		error = cap_check(haverights, needrights);
+		if (error != 0)
+			return (error);
+		if ((needrights & CAP_FCNTL) != 0) {
+			error = cap_fcntl_check(fdp, fd, needfcntl);
 			if (error != 0)
 				return (error);
-			if ((needrights & CAP_FCNTL) != 0) {
-				error = cap_fcntl_check(fdp, fd, needfcntl);
-				if (error != 0)
-					return (error);
-			}
 		}
 #endif
 		count = fp->f_count;
-- 
1.8.1.3

-------------- next part --------------
From d4c8e6bbeb678867b2bd1c0ec09faf60642465d0 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:43:28 +0100
Subject: [PATCH 14/24] Avoide double test.

---
 sys/kern/sys_capability.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index ecb3a6b..dbbda01 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -314,12 +314,12 @@ cap_ioctl_limit_check(struct filedesc *fdp, int fd, const u_long *cmds,
 
 	ocmds = fdp->fd_ofiles[fd].fde_ioctls;
 	for (i = 0; i < ncmds; i++) {
-		for (j = 0; j < oncmds; j++) {
+		for (j = 0;; j++) {
+			if (j == oncmds)
+				return (ENOTCAPABLE);
 			if (cmds[i] == ocmds[j])
 				break;
 		}
-		if (j == oncmds)
-			return (ENOTCAPABLE);
 	}
 
 	return (0);
-- 
1.8.1.3

-------------- next part --------------
From ba10274ab48bf9b2a2e6b8d50c59eb24074e6ec0 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:45:09 +0100
Subject: [PATCH 15/24] Simplify if (x != 0) x = 0; to just x = 0;.

---
 sys/kern/sys_capability.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index dbbda01..153364b 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -232,10 +232,8 @@ sys_cap_rights_limit(struct thread *td, struct cap_rights_limit_args *uap)
 			fdp->fd_ofiles[fd].fde_ioctls = NULL;
 			fdp->fd_ofiles[fd].fde_nioctls = 0;
 		}
-		if ((rights & CAP_FCNTL) == 0 &&
-		    fdp->fd_ofiles[fd].fde_fcntls != 0) {
+		if ((rights & CAP_FCNTL) == 0)
 			fdp->fd_ofiles[fd].fde_fcntls = 0;
-		}
 	}
 	FILEDESC_XUNLOCK(fdp);
 	return (error);
@@ -557,7 +555,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
 		fdp->fd_ofiles[newfd].fde_ioctls = NULL;
 		fdp->fd_ofiles[newfd].fde_nioctls = 0;
 	}
-	if ((rights & CAP_FCNTL) == 0 && fdp->fd_ofiles[newfd].fde_fcntls != 0)
+	if ((rights & CAP_FCNTL) == 0)
 		fdp->fd_ofiles[newfd].fde_fcntls = 0;
 	FILEDESC_XUNLOCK(fdp);
 
-- 
1.8.1.3

-------------- next part --------------
From 38ec55338714e68b845ee45272fac7e7333727ef Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:47:23 +0100
Subject: [PATCH 16/24] Reduce indentation.

---
 sys/kern/kern_descrip.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index d1465a0..612d51a 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2463,15 +2463,13 @@ fgetvp_rights(struct thread *td, int fd, cap_rights_t need,
 	if (error != 0)
 		return (error);
 
-	if (fp->f_vnode == NULL) {
-		error = EINVAL;
-	} else {
-		*vpp = fp->f_vnode;
-		vref(*vpp);
-		filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps);
-	}
+	if (fp->f_vnode == NULL)
+		return EINVAL;
 
-	return (error);
+	*vpp = fp->f_vnode;
+	vref(*vpp);
+	filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps);
+	return (0);
 }
 
 int
-- 
1.8.1.3

-------------- next part --------------
From 64e66277c265b3a1221d9e269c8f33345e43332c Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 13:49:37 +0100
Subject: [PATCH 17/24] Avoid code duplication on error paths.

---
 sys/kern/sys_capability.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index 153364b..e87a4e5 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -344,37 +344,32 @@ sys_cap_ioctls_limit(struct thread *td, struct cap_ioctls_limit_args *uap)
 	} else {
 		cmds = malloc(sizeof(cmds[0]) * ncmds, M_TEMP, M_WAITOK);
 		error = copyin(uap->cmds, cmds, sizeof(cmds[0]) * ncmds);
-		if (error != 0) {
-			free(cmds, M_TEMP);
-			return (error);
-		}
+		if (error != 0)
+			goto out;
 	}
 
 	fdp = td->td_proc->p_fd;
 	FILEDESC_XLOCK(fdp);
 
 	if (fget_locked(fdp, fd) == NULL) {
-		FILEDESC_XUNLOCK(fdp);
-		free(cmds, M_TEMP);
-		return (EBADF);
+		error = EBADF;
+		goto out_locked;
 	}
 
 	error = cap_ioctl_limit_check(fdp, fd, cmds, ncmds);
-	if (error != 0) {
-		FILEDESC_XUNLOCK(fdp);
-		free(cmds, M_TEMP);
-		return (error);
-	}
+	if (error != 0)
+		goto out_locked;
 
 	ocmds = fdp->fd_ofiles[fd].fde_ioctls;
 	fdp->fd_ofiles[fd].fde_ioctls = cmds;
 	fdp->fd_ofiles[fd].fde_nioctls = ncmds;
+	cmds = ocmds;
 
+out_locked:
 	FILEDESC_XUNLOCK(fdp);
-
-	free(ocmds, M_TEMP);
-
-	return (0);
+out:
+	free(cmds, M_TEMP);
+	return (error);
 }
 
 int
@@ -396,8 +391,8 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioctls_get_args *uap)
 	FILEDESC_SLOCK(fdp);
 
 	if (fget_locked(fdp, fd) == NULL) {
-		FILEDESC_SUNLOCK(fdp);
-		return (EBADF);
+		error = EBADF;
+		goto out;
 	}
 
 	/*
@@ -410,19 +405,18 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioctls_get_args *uap)
 	if (cmds != NULL && fdep->fde_ioctls != NULL) {
 		error = copyout(fdep->fde_ioctls, cmds,
 		    sizeof(cmds[0]) * MIN(fdep->fde_nioctls, maxcmds));
-		if (error != 0) {
-			FILEDESC_SUNLOCK(fdp);
-			return (error);
-		}
+		if (error != 0)
+			goto out;
 	}
 	if (fdep->fde_nioctls == -1)
 		td->td_retval[0] = INT_MAX;
 	else
 		td->td_retval[0] = fdep->fde_nioctls;
+	error = 0;
 
+out:
 	FILEDESC_SUNLOCK(fdp);
-
-	return (0);
+	return (error);
 }
 
 /*
-- 
1.8.1.3

-------------- next part --------------
From 0e692e05accd56a4db1f938ad7aa4c50979a6148 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 14:00:01 +0100
Subject: [PATCH 18/24] (Hopefully) slightly improve manpages and comments.

---
 lib/libc/gen/cap_sandboxed.3    |  6 +++---
 lib/libc/sys/cap_fcntls_limit.2 | 14 +++++++-------
 lib/libc/sys/cap_ioctls_limit.2 | 24 ++++++++++++------------
 lib/libc/sys/cap_rights_limit.2 | 10 +++++-----
 sys/sys/capability.h            |  4 ++--
 5 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/lib/libc/gen/cap_sandboxed.3 b/lib/libc/gen/cap_sandboxed.3
index 4896e02..067d6d2 100644
--- a/lib/libc/gen/cap_sandboxed.3
+++ b/lib/libc/gen/cap_sandboxed.3
@@ -44,10 +44,10 @@
 .Fn cap_sandboxed
 returns
 .Va true
-is the process is in a capability mode sandbox or
+if the process is in a capability mode sandbox or
 .Va false
 if it is not.
-This function is more handy alternative to the
+This function is a more handy alternative to the
 .Xr cap_getmode 2
 system call as it always succeeds, so there is no need for error checking.
 If the support for capability mode is not compiled into the kernel,
@@ -57,7 +57,7 @@ will always return
 .Sh RETURN VALUES
 Function
 .Fn cap_sandboxed
-is always successful and can return either
+is always successful and will return either
 .Va true
 or
 .Va false .
diff --git a/lib/libc/sys/cap_fcntls_limit.2 b/lib/libc/sys/cap_fcntls_limit.2
index 38ab9cb..8fa7463 100644
--- a/lib/libc/sys/cap_fcntls_limit.2
+++ b/lib/libc/sys/cap_fcntls_limit.2
@@ -44,15 +44,15 @@
 .Ft int
 .Fn cap_fcntls_get "int fd" "uint32_t *fcntlrightsp"
 .Sh DESCRIPTION
-If a file descriptor is granted
+If a file descriptor is granted the
 .Dv CAP_FCNTL
-capability right, list of allowed
+capability right, the list of allowed
 .Xr fcntl 2
 commands can be selectively reduced (but never expanded) with the
 .Fn cap_fcntls_limit
 system call.
 .Pp
-Bitmask of allowed fcntls commands for a given file descriptor can be obtained
+A bitmask of allowed fcntls commands for a given file descriptor can be obtained
 with the
 .Fn cap_fcntls_get
 system call.
@@ -89,13 +89,13 @@ succeeds unless:
 .It Bq Er EBADF
 The
 .Fa fd
-argument is not a valid active descriptor.
+argument is not a valid descriptor.
 .It Bq Er EINVAL
 An invalid flag has been passed in
 .Fa fcntlrights .
 .It Bq Er ENOTCAPABLE
 .Fa fcntlrights
-would expand list of allowed
+would expand the list of allowed
 .Xr fcntl 2
 commands.
 .El
@@ -106,11 +106,11 @@ succeeds unless:
 .It Bq Er EBADF
 The
 .Fa fd
-argument is not a valid active descriptor.
+argument is not a valid descriptor.
 .It Bq Er EFAULT
 The
 .Fa fcntlrightsp
-argument points at invalid address.
+argument points at an invalid address.
 .El
 .Sh SEE ALSO
 .Xr cap_ioctls_limit 2 ,
diff --git a/lib/libc/sys/cap_ioctls_limit.2 b/lib/libc/sys/cap_ioctls_limit.2
index 34a9bff..de524d7 100644
--- a/lib/libc/sys/cap_ioctls_limit.2
+++ b/lib/libc/sys/cap_ioctls_limit.2
@@ -44,9 +44,9 @@
 .Ft ssize_t
 .Fn cap_ioctls_get "int fd" "unsigned long *cmds" "size_t maxcmds"
 .Sh DESCRIPTION
-If a file descriptor is granted
+If a file descriptor is granted the
 .Dv CAP_IOCTL
-capability right, list of allowed
+capability right, the list of allowed
 .Xr ioctl 2
 commands can be selectively reduced (but never expanded) with the
 .Fn cap_ioctls_limit
@@ -62,21 +62,21 @@ There might be up to
 .Va 256
 elements in the array.
 .Pp
-List of allowed ioctl commands for a given file descriptor can be obtained
+The list of allowed ioctl commands for a given file descriptor can be obtained
 with the
 .Fn cap_ioctls_get
 system call.
 The
 .Fa cmds
-arguments points at the memory that can hold up to
+argument points at memory that can hold up to
 .Fa maxcmds
 values.
-The function populates provided buffer with up to
+The function populates the provided buffer with up to
 .Fa maxcmds
-elements, but always returns total number of ioctl commands allowed for the
+elements, but always returns the total number of ioctl commands allowed for the
 given file descriptor.
-Total number of ioctls commands for the given file descriptor can be obtained
-by passing
+The total number of ioctls commands for the given file descriptor can be
+obtained by passing
 .Dv NULL as the
 .Fa cmds
 argument and
@@ -114,11 +114,11 @@ succeeds unless:
 .It Bq Er EBADF
 The
 .Fa fd
-argument is not a valid active descriptor.
+argument is not a valid descriptor.
 .It Bq Er EFAULT
 The
 .Fa cmds
-argument points at invalid address.
+argument points at an invalid address.
 .It Bq Er EINVAL
 The
 .Fa ncmds
@@ -126,7 +126,7 @@ argument is greater than
 .Va 256 .
 .It Bq Er ENOTCAPABLE
 .Fa cmds
-would expand list of allowed
+would expand the list of allowed
 .Xr ioctl 2
 commands.
 .El
@@ -137,7 +137,7 @@ succeeds unless:
 .It Bq Er EBADF
 The
 .Fa fd
-argument is not a valid active descriptor.
+argument is not a valid descriptor.
 .It Bq Er EFAULT
 The
 .Fa cmds
diff --git a/lib/libc/sys/cap_rights_limit.2 b/lib/libc/sys/cap_rights_limit.2
index 2e83ea0..e2ff134 100644
--- a/lib/libc/sys/cap_rights_limit.2
+++ b/lib/libc/sys/cap_rights_limit.2
@@ -68,7 +68,7 @@ Once capability rights are reduced, operations on the file descriptor will be
 limited to those permitted by
 .Fa rights .
 .Pp
-Bitmask of capability rights assigned to a file descriptor can be obtained with
+A bitmask of capability rights assigned to a file descriptor can be obtained with
 the
 .Fn cap_rights_get
 system call.
@@ -178,7 +178,7 @@ Note that only the
 and
 .Dv F_SETOWN
 commands require this capability right.
-Also note that the list of permitted commands can be futher limited with the
+Also note that the list of permitted commands can be further limited with the
 .Xr cap_fcntls_limit 2
 system call.
 .It Dv CAP_FLOCK
@@ -254,7 +254,7 @@ Permit
 .Xr ioctl 2 .
 Be aware that this system call has enormous scope, including potentially
 global scope for some objects.
-The list of permitted ioctl commands can be futher limited with the
+The list of permitted ioctl commands can be further limited with the
 .Xr cap_ioctls_limit 2
 system call.
 .\" XXXPJD: Doesn't exist anymore.
@@ -470,7 +470,7 @@ with the
 .Dv O_WRONLY
 flag, but without the
 .Dv O_APPEND
-flag
+flag,
 .Dv CAP_SEEK
 is also required.
 .El
@@ -503,7 +503,7 @@ argument is not a valid active descriptor.
 .It Bq Er EFAULT
 The
 .Fa rightsp
-argument points at invalid address.
+argument points at an invalid address.
 .El
 .Sh SEE ALSO
 .Xr accept 2 ,
diff --git a/sys/sys/capability.h b/sys/sys/capability.h
index 9eed4d1..130d200 100644
--- a/sys/sys/capability.h
+++ b/sys/sys/capability.h
@@ -267,7 +267,7 @@ int	cap_enter(void);
 
 /*
  * Are we sandboxed (in capability mode)?
- * This is libc wrapper around cap_getmode(2) system call.
+ * This is a libc wrapper around the cap_getmode(2) system call.
  */
 _Bool	cap_sandboxed(void);
 
@@ -289,7 +289,7 @@ int cap_rights_get(int fd, cap_rights_t *rightsp);
  */
 int cap_ioctls_limit(int fd, const unsigned long *cmds, size_t ncmds);
 /*
- * Returns array of allowed ioctls for the given descriptors.
+ * Returns array of allowed ioctls for the given descriptor.
  * If all ioctls are allowed, the cmds array is not populated and
  * the function returns INT_MAX.
  */
-- 
1.8.1.3

-------------- next part --------------
From 2b8a154312f692912fc90bc2acd8e55ab1fc4ba4 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 14:00:31 +0100
Subject: [PATCH 19/24] Sort Xr.

---
 lib/libc/sys/cap_rights_limit.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/libc/sys/cap_rights_limit.2 b/lib/libc/sys/cap_rights_limit.2
index e2ff134..d8d8777 100644
--- a/lib/libc/sys/cap_rights_limit.2
+++ b/lib/libc/sys/cap_rights_limit.2
@@ -546,8 +546,8 @@ argument points at an invalid address.
 .Xr mq_open 2 ,
 .Xr open 2 ,
 .Xr openat 2 ,
-.Xr pdgetpid 2 ,
 .Xr pdfork 2 ,
+.Xr pdgetpid 2 ,
 .Xr pdkill 2 ,
 .Xr pdwait4 2 ,
 .Xr pipe 2 ,
-- 
1.8.1.3

-------------- next part --------------
From 42aaaa14c5c1a5d96503a88a381909d6c3254727 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 14:04:36 +0100
Subject: [PATCH 20/24] Avoid comparing signed with unsigned values.

---
 sys/fs/nfsserver/nfs_nfsdport.c | 2 +-
 sys/kern/sys_capability.c       | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c
index 880f965..69f5629 100644
--- a/sys/fs/nfsserver/nfs_nfsdport.c
+++ b/sys/fs/nfsserver/nfs_nfsdport.c
@@ -2775,7 +2775,7 @@ fp_getfvp(struct thread *p, int fd, struct file **fpp, struct vnode **vpp)
 	int error = 0;
 
 	fdp = p->td_proc->p_fd;
-	if ((u_int)fd >= fdp->fd_nfiles ||
+	if (fd < 0 || fdp->fd_nfiles <= fd ||
 	    (fp = fdp->fd_ofiles[fd].fde_file) == NULL) {
 		error = EBADF;
 		goto out;
diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index e87a4e5..e3622b0 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -274,7 +274,7 @@ cap_ioctl_check(struct filedesc *fdp, int fd, u_long cmd)
 {
 	u_long *cmds;
 	ssize_t ncmds;
-	u_int i;
+	ssize_t i;
 
 	FILEDESC_LOCK_ASSERT(fdp);
 	KASSERT(fd >= 0 && fd < fdp->fd_nfiles,
@@ -302,12 +302,13 @@ cap_ioctl_limit_check(struct filedesc *fdp, int fd, const u_long *cmds,
 {
 	u_long *ocmds;
 	ssize_t oncmds;
-	u_int i, j;
+	size_t i;
+	ssize_t j;
 
 	oncmds = fdp->fd_ofiles[fd].fde_nioctls;
 	if (oncmds == -1)
 		return (0);
-	if (oncmds < ncmds)
+	if ((size_t)oncmds < ncmds)
 		return (ENOTCAPABLE);
 
 	ocmds = fdp->fd_ofiles[fd].fde_ioctls;
-- 
1.8.1.3

-------------- next part --------------
From a05144e19411d2b3fc8716e72ef2fad7cc9449ae Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 15:09:16 +0100
Subject: [PATCH 21/24] For readability simplify if (x) y = a; else y = b; with
 long y to y = x ? a : b.

---
 sys/kern/kern_descrip.c   | 10 +++-------
 sys/kern/sys_capability.c |  6 ++----
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 612d51a..30dac04 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -842,13 +842,9 @@ do_dup(struct thread *td, int flags, int old, int new,
 	fdp->fd_ofiles[new] = fdp->fd_ofiles[old];
 	filecaps_copy(&fdp->fd_ofiles[old].fde_caps,
 	    &fdp->fd_ofiles[new].fde_caps);
-	if ((flags & DUP_CLOEXEC) != 0) {
-		fdp->fd_ofiles[new].fde_flags =
-		    fdp->fd_ofiles[old].fde_flags | UF_EXCLOSE;
-	} else {
-		fdp->fd_ofiles[new].fde_flags =
-		    fdp->fd_ofiles[old].fde_flags & ~UF_EXCLOSE;
-	}
+	fdp->fd_ofiles[new].fde_flags = flags & DUP_CLOEXEC ?
+	    fdp->fd_ofiles[old].fde_flags |  UF_EXCLOSE :
+	    fdp->fd_ofiles[old].fde_flags & ~UF_EXCLOSE;
 	if (new > fdp->fd_lastfile)
 		fdp->fd_lastfile = new;
 	*retval = new;
diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index e3622b0..2306811 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -409,10 +409,8 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioctls_get_args *uap)
 		if (error != 0)
 			goto out;
 	}
-	if (fdep->fde_nioctls == -1)
-		td->td_retval[0] = INT_MAX;
-	else
-		td->td_retval[0] = fdep->fde_nioctls;
+	td->td_retval[0] =
+	    fdep->fde_nioctls != -1 ? fdep->fde_nioctls : INT_MAX;
 	error = 0;
 
 out:
-- 
1.8.1.3

-------------- next part --------------
From 1258951ef3cdb8b8624e6a7032036e0e5e0ac8c6 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 15:23:15 +0100
Subject: [PATCH 22/24] Unify and simplify bitset inclusion tests.

- (have | need) != have looks like a typo: shouldn't the | be a &?.
- some places used (have | need) != have, others (have & need) == need.
- (need & ~have) != 0 -- need and not have -- is easier to comprehend.
- Avoid duplication, especially when the duplicated subexpression is long.
---
 sys/kern/kern_descrip.c           |  4 ++--
 sys/kern/sys_capability.c         | 11 +++++------
 usr.bin/procstat/procstat_files.c |  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 30dac04..64176ca 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1422,9 +1422,9 @@ static void
 filecaps_validate(const struct filecaps *fcaps, const char *func)
 {
 
-	KASSERT((fcaps->fc_rights | CAP_MASK_VALID) == CAP_MASK_VALID,
+	KASSERT((fcaps->fc_rights & ~CAP_MASK_VALID) == 0,
 	    ("%s: invalid rights", func));
-	KASSERT((fcaps->fc_fcntls | CAP_FCNTL_ALL) == CAP_FCNTL_ALL,
+	KASSERT((fcaps->fc_fcntls & ~CAP_FCNTL_ALL) == 0,
 	    ("%s: invalid fcntls", func));
 	KASSERT(fcaps->fc_fcntls == 0 || (fcaps->fc_rights & CAP_FCNTL) != 0,
 	    ("%s: fcntls without CAP_FCNTL", func));
diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c
index 2306811..d06918c 100644
--- a/sys/kern/sys_capability.c
+++ b/sys/kern/sys_capability.c
@@ -148,7 +148,7 @@ static inline int
 _cap_check(cap_rights_t have, cap_rights_t need, enum ktr_cap_fail_type type)
 {
 
-	if ((have | need) != have) {
+	if ((need & ~have) != 0) {
 #ifdef KTRACE
 		if (KTRPOINT(curthread, KTR_CAPFAIL))
 			ktrcapfail(type, need, have);
@@ -215,7 +215,7 @@ sys_cap_rights_limit(struct thread *td, struct cap_rights_limit_args *uap)
 	AUDIT_ARG_FD(fd);
 	AUDIT_ARG_RIGHTS(rights);
 
-	if ((rights | CAP_ALL) != CAP_ALL)
+	if ((rights & ~CAP_ALL) != 0)
 		return (EINVAL);
 
 	fdp = td->td_proc->p_fd;
@@ -452,7 +452,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fcntls_limit_args *uap)
 	AUDIT_ARG_FD(fd);
 	AUDIT_ARG_FCNTL_RIGHTS(fcntlrights);
 
-	if ((fcntlrights | CAP_FCNTL_ALL) != CAP_FCNTL_ALL)
+	if ((fcntlrights & ~CAP_FCNTL_ALL) != 0)
 		return (EINVAL);
 
 	fdp = td->td_proc->p_fd;
@@ -463,8 +463,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fcntls_limit_args *uap)
 		return (EBADF);
 	}
 
-	if ((fdp->fd_ofiles[fd].fde_fcntls | fcntlrights) !=
-	    fdp->fd_ofiles[fd].fde_fcntls) {
+	if ((fcntlrights & ~fdp->fd_ofiles[fd].fde_fcntls) != 0) {
 		FILEDESC_XUNLOCK(fdp);
 		return (ENOTCAPABLE);
 	}
@@ -515,7 +514,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
 	AUDIT_ARG_FD(fd);
 	AUDIT_ARG_RIGHTS(rights);
 
-	if ((rights | CAP_ALL) != CAP_ALL)
+	if ((rights & ~CAP_ALL) != 0)
 		return (EINVAL);
 
 	fdp = td->td_proc->p_fd;
diff --git a/usr.bin/procstat/procstat_files.c b/usr.bin/procstat/procstat_files.c
index eefc5b7..16dab0b 100644
--- a/usr.bin/procstat/procstat_files.c
+++ b/usr.bin/procstat/procstat_files.c
@@ -243,7 +243,7 @@ width_capability(cap_rights_t rights)
 	count = 0;
 	width = 0;
 	for (i = 0; i < cap_desc_count; i++) {
-		if ((rights & cap_desc[i].cd_right) == cap_desc[i].cd_right) {
+		if ((cap_desc[i].cd_right & ~rights) == 0) {
 			width += strlen(cap_desc[i].cd_desc);
 			if (count)
 				width++;
@@ -267,7 +267,7 @@ print_capability(cap_rights_t rights, u_int capwidth)
 			printf("-");
 	}
 	for (i = 0; i < cap_desc_count; i++) {
-		if ((rights & cap_desc[i].cd_right) == cap_desc[i].cd_right) {
+		if ((cap_desc[i].cd_right & ~rights) == 0) {
 			printf("%s%s", count ? "," : "", cap_desc[i].cd_desc);
 			width += strlen(cap_desc[i].cd_desc);
 			if (count)
-- 
1.8.1.3

-------------- next part --------------
From 4d721b7aa909091fc705cc8f822a13da69e1954c Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 15:33:48 +0100
Subject: [PATCH 23/24] Simplify assertion condition, which contains a
 duplicated subexpression.

---
 sys/kern/kern_descrip.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 64176ca..c2556e2 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1428,9 +1428,8 @@ filecaps_validate(const struct filecaps *fcaps, const char *func)
 	    ("%s: invalid fcntls", func));
 	KASSERT(fcaps->fc_fcntls == 0 || (fcaps->fc_rights & CAP_FCNTL) != 0,
 	    ("%s: fcntls without CAP_FCNTL", func));
-	KASSERT(((fcaps->fc_nioctls == -1 || fcaps->fc_nioctls == 0) &&
-	         fcaps->fc_ioctls == NULL) ||
-	        (fcaps->fc_nioctls > 0 && fcaps->fc_ioctls != NULL),
+	KASSERT(fcaps->fc_ioctls != NULL ? fcaps->fc_nioctls > 0 :
+	    (fcaps->fc_nioctls == -1 || fcaps->fc_nioctls == 0),
 	    ("%s: invalid ioctls", func));
 	KASSERT(fcaps->fc_nioctls == 0 || (fcaps->fc_rights & CAP_IOCTL) != 0,
 	    ("%s: ioctls without CAP_IOCTL", func));
-- 
1.8.1.3

-------------- next part --------------
From 98dd3c8a988e043b4a8f02ed4c19d39d30e1145d Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sun, 24 Feb 2013 15:47:53 +0100
Subject: [PATCH 24/24] Factorise code to free a file descriptor.

---
 sys/kern/kern_descrip.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index c2556e2..8256501 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -287,6 +287,19 @@ fdunused(struct filedesc *fdp, int fd)
 }
 
 /*
+ * Free a file descriptor.
+ */
+static void
+fdfree(struct filedesc *fdp, int fd)
+{
+	struct filedescent *fde = fdp->fd_ofiles[fd];
+
+	filecaps_free(&fde->fde_caps);
+	bzero(fde, sizeof(*fde));
+	fdunused(fdp, fd);
+}
+
+/*
  * System calls on descriptors.
  */
 #ifndef _SYS_SYSPROTO_H_
@@ -1165,9 +1178,7 @@ kern_close(td, fd)
 		FILEDESC_XUNLOCK(fdp);
 		return (EBADF);
 	}
-	filecaps_free(&fdp->fd_ofiles[fd].fde_caps);
-	bzero(&fdp->fd_ofiles[fd], sizeof(fdp->fd_ofiles[fd]));
-	fdunused(fdp, fd);
+	fdfree(fdp, fd);
 
 	/* closefp() drops the FILEDESC lock for us. */
 	return (closefp(fdp, fd, fp, td, 1));
@@ -2035,9 +2046,7 @@ setugidsafety(struct thread *td)
 			 * NULL-out descriptor prior to close to avoid
 			 * a race while close blocks.
 			 */
-			filecaps_free(&fdp->fd_ofiles[i].fde_caps);
-			bzero(&fdp->fd_ofiles[i], sizeof(fdp->fd_ofiles[i]));
-			fdunused(fdp, i);
+			fdfree(fdp, i);
 			FILEDESC_XUNLOCK(fdp);
 			(void) closef(fp, td);
 			FILEDESC_XLOCK(fdp);
@@ -2059,9 +2068,7 @@ fdclose(struct filedesc *fdp, struct file *fp, int idx, struct thread *td)
 
 	FILEDESC_XLOCK(fdp);
 	if (fdp->fd_ofiles[idx].fde_file == fp) {
-		filecaps_free(&fdp->fd_ofiles[idx].fde_caps);
-		bzero(&fdp->fd_ofiles[idx], sizeof(fdp->fd_ofiles[idx]));
-		fdunused(fdp, idx);
+		fdfree(fdp, idx);
 		FILEDESC_XUNLOCK(fdp);
 		fdrop(fp, td);
 	} else
@@ -2094,9 +2101,7 @@ fdcloseexec(struct thread *td)
 		fp = fde->fde_file;
 		if (fp != NULL && (fp->f_type == DTYPE_MQUEUE ||
 		    (fde->fde_flags & UF_EXCLOSE))) {
-			filecaps_free(&fde->fde_caps);
-			bzero(fde, sizeof(*fde));
-			fdunused(fdp, i);
+			fdfree(fdp, i);
 			(void) closefp(fdp, i, fp, td, 0);
 			/* closefp() drops the FILEDESC lock. */
 			FILEDESC_XLOCK(fdp);
-- 
1.8.1.3



More information about the freebsd-arch mailing list