Date: Sun, 24 Feb 2013 16:07:12 +0100 From: Christoph Mallon <christoph.mallon@gmx.de> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: Large Capsicum patch for review. Message-ID: <512A2CA0.2050509@gmx.de> In-Reply-To: <5129ADC5.5040306@gmx.de> References: <20130213025547.GA2025@garage.freebsd.pl> <20130213230221.GB1375@garage.freebsd.pl> <20130223221116.GR1377@garage.freebsd.pl> <5129ADC5.5040306@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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 [-- Attachment #2 --] From 7f18186c451c235bcc3dbf2e25f65613e1ce7f16 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #3 --] From 7ad09a0cf0ceb9828171133e32f7148ffde62d97 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #4 --] From ae6ac8b82374e83dcc2bda7dd6e17da5d65d9eae Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #5 --] From 4822faec8ea5c53c673740eeec0cb604960af37b Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #6 --] From 1d1defdf7dd117afa56330c958daeac5d52be05f Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #7 --] From 5107ab9aec6766c9ce5285939637c5739b2aed0c Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #8 --] From d2fe7a6f9d8e08943600b1f5d6a2818c6c4aa794 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #9 --] From cc2de42e944828943c17fcba2a821becc634b9a4 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #10 --] From 2b9add7396a9485add6a4246071cf041a50161d1 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #11 --] From 4645cc6db54f29c1f5ec32c47794fd135f99cf60 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #12 --] From fc8878d0a43ea3f9d77b1fd51f90f4b4be188fb2 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #13 --] From 41fc1f39a18331f57e6a03b54f4b78cbbd123cd5 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #14 --] From d4c8e6bbeb678867b2bd1c0ec09faf60642465d0 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #15 --] From ba10274ab48bf9b2a2e6b8d50c59eb24074e6ec0 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #16 --] From 38ec55338714e68b845ee45272fac7e7333727ef Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #17 --] From 64e66277c265b3a1221d9e269c8f33345e43332c Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #18 --] From 0e692e05accd56a4db1f938ad7aa4c50979a6148 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #19 --] From 2b8a154312f692912fc90bc2acd8e55ab1fc4ba4 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #20 --] From 42aaaa14c5c1a5d96503a88a381909d6c3254727 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #21 --] From a05144e19411d2b3fc8716e72ef2fad7cc9449ae Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #22 --] From 1258951ef3cdb8b8624e6a7032036e0e5e0ac8c6 Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #23 --] From 4d721b7aa909091fc705cc8f822a13da69e1954c Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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 [-- Attachment #24 --] From 98dd3c8a988e043b4a8f02ed4c19d39d30e1145d Mon Sep 17 00:00:00 2001 From: Christoph Mallon <christoph.mallon@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?512A2CA0.2050509>