fcntl error

fcntl error

[David Howells]

Hi Andrew, Linus,

The attached patch fixes a minor problem with fcntl.

get_close_on_exec() uses FD_ISSET() to determine the fd state. However,
FD_ISSET() does not return 0 or 1 on all archs. On some it returns 0 or non-0,
which is fine by POSIX.

Also, the argument of set_close_on_exec() is being AND'ed with literal 1. This
is incorrect - there's no requirement for FD_CLOEXEC to be 1.

This is also wrong on 2.4 kernels.

David


--- fs/fcntl.c.orig     2004-03-18 16:29:57.000000000 +0000
+++ fs/fcntl.c  2004-03-18 16:30:27.000000000 +0000
@@ -293,11 +293,11 @@
                        err = dupfd(filp, arg);
                        break;
                case F_GETFD:
-                       err = get_close_on_exec(fd);
+                       err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
                        break;
                case F_SETFD:
                        err = 0;
-                       set_close_on_exec(fd, arg&1);
+                       set_close_on_exec(fd, arg & FD_CLOEXEC);
                        break;
                case F_GETFL:
                        err = filp->f_flags;

Re: fcntl error

[Linus Torvalds]

On Thu, 18 Mar 2004, David Howells wrote:
> 
> The attached patch fixes a minor problem with fcntl.

I agree that it is a cleanup, but I disagree on the "problem" part.

> get_close_on_exec() uses FD_ISSET() to determine the fd state. However,
> FD_ISSET() does not return 0 or 1 on all archs. On some it returns 0 or non-0,
> which is fine by POSIX.

FD_ISSET() is broken if it returns anything but 0/1, in my not-so-humble 
opinion.

Looking at the implementations, you are right that some architectures 
don't do this right, but that is a bug, and it's a bug in FD_ISSET(), not 
in fcntl.

The fact is, FD_ISSET() isn't always used in just as a conditional, and
you're supposed to be able to do

	int was_set = FD_ISSET(..);
	...

and in fact I'd suggest very _strongly_ that it also should work with

	bool is_set = FD_ISSET(..);

where some people use "char" for booleans for space reasons.

That implies that while non-zero for "set" is ok, that non-zero had better
have the _low_ bits set. Which is not true on architectures that use just
a logical and with the bits in the word.

Which implies that FD_ISSET() really must NOT be of that "logical and" 
approach, which in turn implies that it should be either a inequality 
expression, or it should be a "shift down and then and with 1".

And in both of those cases, the result ends up being 0/1. So we might as 
well just make it so.

In short, the real bug is elsewhere.

> Also, the argument of set_close_on_exec() is being AND'ed with literal 1. This
> is incorrect - there's no requirement for FD_CLOEXEC to be 1.

Not in theory, no. In practice, it always is.

I'd suggest architecture maintainers fix their __FD_ISSET() 
implementations to conform to the proper return value.

		Linus

Re: fcntl error

[Andreas Schwab]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 18 Mar 2004, David Howells wrote:
>> 
>> The attached patch fixes a minor problem with fcntl.
>
> I agree that it is a cleanup, but I disagree on the "problem" part.
>
>> get_close_on_exec() uses FD_ISSET() to determine the fd state. However,
>> FD_ISSET() does not return 0 or 1 on all archs. On some it returns 0 or non-0,
>> which is fine by POSIX.
>
> FD_ISSET() is broken if it returns anything but 0/1, in my not-so-humble 
> opinion.

POSIX clearly says that _any_ non-zero value is ok, similar to the ctype.h
functions.  Of course, the kernel can set different standards internally.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: fcntl error

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:
> That implies that while non-zero for "set" is ok, that non-zero had better
> have the _low_ bits set. Which is not true on architectures that use just
> a logical and with the bits in the word.
> 
> Which implies that FD_ISSET() really must NOT be of that "logical and" 
> approach, which in turn implies that it should be either a inequality 
> expression, or it should be a "shift down and then and with 1".

Or maybe:

#define	__FD_ISSET(d, set)	(!!((set)->fds_bits[__FDELT(d)] & __FDMASK(d)))

Which is probably better than a shift and bit-and. That then leaves it up to
the compiler as to whether the generation of 0 or 1 is actually necessary
(which if isn't if it's just the condition in an if-statement).

Or even:

#define FD_ISSET(fd,fdsetp)	(!!__FD_ISSET(fd,fdsetp))

Which would then apply to all arch's regardless.

David