[PATCH]: possible fix for the fifoor problem

[Bruce Evans]

On Tue, 7 Nov 2006, Jung-uk Kim wrote:

> On Tuesday 07 November 2006 12:19 pm, Divacky Roman wrote:
>> the patch is wrong.... this forces NONBLOCKing on all opened files
>> which is wrong.
>
> Nope.  It does not force anything.

But it does.  open() has side effects for some file types, and does have
signifcant side effects for fifos.  The original problem is caused by the
side effect of blocking.  Opening for writing or read/write would have
the side effect of unblocking all readers blocked in open() and letting
their open() complete.  Opening for reading gives more delicate state
changes which might be harmless, but this is not clear.

> static void
> translate_path_major_minor(struct thread *td, char *path, struct stat *buf)
> {
> 	struct proc *p = td->td_proc;
> 	struct filedesc *fdp = p->p_fd;
> 	struct file *fp;
> 	int fd;
> 	int temp;
>
> 	temp = td->td_retval[0];
> 	if (kern_open(td, path, UIO_SYSSPACE, O_RDONLY, 0) != 0)
> 		return;
> 	fd = td->td_retval[0];
> 	td->td_retval[0] = temp;
> 	translate_fd_major_minor(td, fd, buf);
> 	FILEDESC_LOCK(fdp);
> 	fp = fdp->fd_ofiles[fd];
> 	FILEDESC_UNLOCK(fdp);
> 	fdclose(fdp, fdp->fd_ofiles[fd], fd, td);
> }
>
> As you can see the function is only used internally to convert
> major/minor and fd is closed at the end of the function.

Anything more than a stat() is too dangerous.

>> according to a comment in linux source code linux never blocks for
>> O_RDWR which is what I tried to implement with my patch
>
> We don't advertise it but we do the same, AFAIK. ;-)

I think this happens automatically.  The reader would block without
O_NONBLOCK or a writer, but O_RDWR gives a writer so the only possible
block is a transient internal one if the implementation opens the reader
first.  The thread doing the O_RDWR open() couldn't see this, but other
threads might be able to see it if the implementation doesn't hold a
lock throughout the open().

But utility functions cannot use O_RDWR due to its side effect.

BTW, I never got around to committing the following workaround for a
related bug in freopen(), since I'm not happy with the patch though I
have been using it for about 10 years:

%%%
Index: freopen.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/stdio/freopen.c,v
retrieving revision 1.13
diff -u -2 -r1.13 freopen.c
--- freopen.c	22 May 2004 15:19:41 -0000	1.13
+++ freopen.c	23 May 2004 04:01:46 -0000
@@ -64,4 +64,5 @@
  	FILE *fp;
  {
+	struct stat st;
  	int f;
  	int dflags, flags, isopen, oflags, sverrno, wantfd;
@@ -137,4 +138,7 @@
  	 * a descriptor, defer closing it; freopen("/dev/stdin", "r", stdin)
  	 * should work.  This is unnecessary if it was not a Unix file.
+	 * However, not closing the descriptor is a bug if closing it would
+	 * have side effects.  We close fifos because completely closing a
+	 * fifo flushes it, and the NIST POSIX test suite tests for this.
  	 */
  	if (fp->_flags == 0) {
@@ -148,5 +152,9 @@
  		/* if close is NULL, closing is a no-op, hence pointless */
  		isopen = fp->_close != NULL;
-		if ((wantfd = fp->_file) < 0 && isopen) {
+		wantfd = fp->_file;
+		if (isopen &&
+		    (wantfd < 0 ||
+		     (_fstat(wantfd, &st) == 0 && S_ISFIFO(st.st_mode)
+		      && st.st_ino != 0))) {
  			(void) (*fp->_close)(fp->_cookie);
  			isopen = 0;
%%%

stat() can be used similarly to give a workaround (that I'm not happy
with) for translate_path_major_minor(): first stat() the path, and
don't do anything unless it is a cdev or maybe a bdev
(translate_fd_major_minor() still supports bdevs).  This avoids the problem
for fifos.  However, cdevs must be handled, and opening of cdevs can have
harmmful side effects (e.g., rewinding of tape drives).  My fix for
freopen() is incomplete because it doesn't handle the opposite problem.
E.g., the missing close() might break setting of file marks on tapes.
The problem is just smaller for freopen() since stdio shouldn't be used
on tape drives, etc., while the translation function is used a lot and
doesn't require much privilege.

Bruce