svn commit: r272505 - in head/sys: kern sys

Bruce Evans brde at optusnet.com.au
Sun Oct 5 01:52:18 UTC 2014


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

> On 04 Oct 2014, at 08:08 , Mateusz Guzik <mjg at 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


More information about the svn-src-head mailing list