On Sat, 4 Oct 2014, Bjoern A. Zeeb wrote:

On 04 Oct 2014, at 08:08 , Mateusz Guzik <m...@freebsd.org> wrote:
...
Log:
 Plug capability races.
...

This file is included from user space.  There is no opt_capsicum.h there.
Including an opt_* in the header file seems wrong in a lot of ways usually.

I tried to add a bandaid for the moment with r272523 which (to be honest) makes 
it worse.

This needs a better fix.

I also wonder why the (conditional) fde_seq ended up at the beginning of the 
structure rather than the end?

It adds to the already-massive namespace pollution in other ways.

Modified: head/sys/sys/filedesc.h
==============================================================================
--- head/sys/sys/filedesc.h     Sat Oct  4 08:05:39 2014        (r272504)
+++ head/sys/sys/filedesc.h     Sat Oct  4 08:08:56 2014        (r272505)
@@ -33,11 +33,14 @@
#ifndef _SYS_FILEDESC_H_
#define _SYS_FILEDESC_H_

+#include "opt_capsicum.h"
+

Fatal namspace pollution.

#include <sys/caprights.h>
#include <sys/queue.h>
#include <sys/event.h>
#include <sys/lock.h>
#include <sys/priority.h>

Old massive namespace pollution.

+#include <sys/seq.h>

New namespace pollution.

#include <sys/sx.h>

#include <machine/_limits.h>

Old massive namespace pollution, continued.

The pollution is nested.  <sys/sx.h> at least has a _KERNEL ifdef around
most of its pollution.

@@ -50,6 +53,9 @@ struct filecaps {
};

struct filedescent {
+#ifdef CAPABILITIES
+       seq_t            fde_seq;               /* if you need fde_file and 
fde_caps in sync */
+#endif

This ifdef makes the struct not really a struct.  The layout depends on
visible options, and the visible options may depend on pollution.

The worst sort of pollution bug would occur if something else has a
similar ifdef but doesn't include the options header.  Then when different
pollution there results in including this header, the include of the
options header here gives different visible options and thus a different
struct layout there.

        struct file     *fde_file;              /* file structure for open file 
*/
        struct filecaps  fde_caps;              /* per-descriptor rights */
        uint8_t          fde_flags;             /* per-process open file flags 
*/
@@ -58,6 +64,13 @@ struct filedescent {
#define fde_fcntls      fde_caps.fc_fcntls
#define fde_ioctls      fde_caps.fc_ioctls
#define fde_nioctls     fde_caps.fc_nioctls
+#ifdef CAPABILITIES
+#define        fde_change(fde) ((char *)(fde) + sizeof(seq_t))
+#define        fde_change_size (sizeof(struct filedescent) - sizeof(seq_t))
+#else
+#define        fde_change(fde) ((fde))
+#define        fde_change_size (sizeof(struct filedescent))
+#endif

/*
 * This structure is used for the management of descriptors.  It may be
@@ -82,6 +95,9 @@ struct filedesc {
        int     fd_holdleaderscount;    /* block fdfree() for shared close() */
        int     fd_holdleaderswakeup;   /* fdfree() needs wakeup */
};
+#ifdef CAPABILITIES
+#define        fd_seq(fdp, fd) (&(fdp)->fd_ofiles[(fd)].fde_seq)
+#endif

The Ifdefs of the macros are not even wrong.  Macros are not as
polluting as inline functions, so always defining them doen't cause
many problems.  They just give relatively minor namespace pollution
and are unusable if the headers needed to use them are not included.

The patch has some style bugs, e.g., long lines.

Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to