[PATCH]: possible fix for the fifoor problem

[Jung-uk Kim]

On Tuesday 07 November 2006 07:16 pm, Bruce Evans wrote:
> 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.

Ah, you are correct.  I had to think more thoroughly.

> > 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.

Yeah, it is really ugly hack already.

> >> 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.

Thanks for the explanation.

> 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.

Interesting...  I knew the proper fix would be harder than my one-line 
patch. ;-)

Thanks,

Jung-uk Kim