[comp.bugs.4bsd] Bug in file read/write with forked file-descriptors

bvs@light.uucp (Bakul Shah) (09/22/89)

The attached program fails most of the time on V7, 4.3 BSD & Sun 3
systems; which seems to indicate the bug was inherited from V7.
(SVR3 seems to handle it right -- I haven't tried others).

Upon forking file-descriptors are shared by the parent & child.
But if a shared fd is to a regular file and both read from it at
the same time, they get the *same* data instead of serial pieces
of the file (also, the next few bytes are skipped).  There is a
similar problem with write.  The problem is where fp->f_offset is
updated once read/write is finished.  It should be updated while
the inode is locked.

-- Bakul Shah <..!{ames,sun,ucbvax,uunet}!amdcad!light!bvs>

#! /bin/sh
cat > x.c <<EOF
#include <stdio.h>

extern void * malloc();

main(argc, argv)
	int argc;
	char * * argv;
{
	char * buf;
	int count, f, g;
	unsigned int size;

	/* default to stdin if no argv[1] or it is "-" */
	f = (argc > 1 && strcmp(argv[1], "-")) ? open(argv[1]) : 0;
	if (f < 0) { perror(argv[1]); exit(1); }

	/* default to stdout if no argv[2] or it is "-" */
	g = (argc > 2 && strcmp(argv[2], "-")) ? creat(argv[2], 0666) : 1;
	if (g < 0) { perror(argv[2]); exit(1); }

	/* default to 1 byte buffer if no argv[3] */
	size = argc > 3 ? atoi(argv[3]) : 1;
	buf = (char *)malloc(size);
	if (!buf) { fprintf(stderr, "not enough space\n"); exit(1); }

	fork();
	while ((count = read(f, buf, size)) > 0)
		write(g, buf, count);
}
EOF

cc x.c

# The correct output of a.out will be a permutation of input.
# On a V7 system pipes seem to have the same bug.
a.out x.c

chris@mimsy.UUCP (Chris Torek) (09/23/89)

In article <1989Sep22.160808.1407@light.uucp> bvs@light.uucp (Bakul Shah)
writes:
>The problem is where fp->f_offset is updated once read/write is finished.
>It should be updated while the inode is locked.

This bug was fixed quite some time ago (perhaps in 4.3-tahoe, perhaps a
bit afterward).  Here is the fix.  Your lines numbers may not match.

*** /tmp/,RCSt1003823	Sat Sep 23 00:28:18 1989
--- /tmp/,RCSt2003823	Sat Sep 23 00:28:21 1989
***************
*** 4,8 ****
   * specifies the terms and conditions for redistribution.
   *
!  *	@(#)sys_inode.c	7.1 (Berkeley) 6/5/86
   */
  
--- 4,8 ----
   * specifies the terms and conditions for redistribution.
   *
!  *	@(#)sys_inode.c	7.5.1.1 (Berkeley) 11/24/87
   */
  
***************
*** 36,49 ****
  {
  	register struct inode *ip = (struct inode *)fp->f_data;
! 	int error;
  
! 	if ((ip->i_mode&IFMT) == IFREG) {
  		ILOCK(ip);
! 		if (fp->f_flag&FAPPEND && rw == UIO_WRITE)
! 			uio->uio_offset = fp->f_offset = ip->i_size;
! 		error = rwip(ip, uio, rw);
  		IUNLOCK(ip);
- 	} else
- 		error = rwip(ip, uio, rw);
  	return (error);
  }
--- 36,53 ----
  {
  	register struct inode *ip = (struct inode *)fp->f_data;
! 	int count, error;
  
! 	if ((ip->i_mode&IFMT) != IFCHR)
  		ILOCK(ip);
! 	if ((ip->i_mode&IFMT) == IFREG &&
! 	    (fp->f_flag&FAPPEND) &&
! 	    rw == UIO_WRITE)
! 		fp->f_offset = ip->i_size;
! 	uio->uio_offset = fp->f_offset;
! 	count = uio->uio_resid;
! 	error = rwip(ip, uio, rw);
! 	fp->f_offset += count - uio->uio_resid;
! 	if ((ip->i_mode&IFMT) != IFCHR)
  		IUNLOCK(ip);
  	return (error);
  }
***************
*** 148,152 ****
  			bn = fsbtodb(fs,
  			    bmap(ip, lbn, rw == UIO_WRITE ? B_WRITE: B_READ,
! 				(int)(on+n)));
  			if (u.u_error || rw == UIO_WRITE && (long)bn < 0)
  				return (u.u_error);
--- 152,156 ----
  			bn = fsbtodb(fs,
  			    bmap(ip, lbn, rw == UIO_WRITE ? B_WRITE: B_READ,
! 				(int)(on + n)));
  			if (u.u_error || rw == UIO_WRITE && (long)bn < 0)
  				return (u.u_error);
***************
*** 253,256 ****
--- 257,261 ----
  			    fp->f_flag));
  	}
+ 	/* NOTREACHED */
  }
  
***************
*** 319,323 ****
  	register struct file *fp;
  {
! 	register struct inode *ip = (struct inode *)fp->f_data;
  	dev_t dev;
  	int flag, mode;
--- 324,328 ----
  	register struct file *fp;
  {
! 	struct inode *ip = (struct inode *)fp->f_data;
  	dev_t dev;
  	int flag, mode;
***************
*** 331,335 ****
  	 * will prevent close.
  	 */
! 	fp->f_data = (caddr_t) 0;		/* XXX */
  	dev = (dev_t)ip->i_rdev;
  	mode = ip->i_mode & IFMT;
--- 336,340 ----
  	 * will prevent close.
  	 */
! 	fp->f_data = (caddr_t) 0;
  	dev = (dev_t)ip->i_rdev;
  	mode = ip->i_mode & IFMT;
***************
*** 364,368 ****
  		/*
  		 * We don't want to really close the device if it is mounted
! 		 * of if we're swapping on it.
  		 */
  /* MOUNT TABLE SHOULD HOLD INODE */
--- 369,373 ----
  		/*
  		 * We don't want to really close the device if it is mounted
! 		 * or if we're swapping on it.
  		 */
  /* MOUNT TABLE SHOULD HOLD INODE */
-- 
In-Real-Life: Chris Torek, Univ of MD Comp Sci Dept (+1 301 454 7163)
Domain:	chris@mimsy.umd.edu	Path:	uunet!mimsy!chris

chris@mimsy.UUCP (Chris Torek) (09/24/89)

In article <19757@mimsy.UUCP> I wrote:
>This bug was fixed quite some time ago (perhaps in 4.3-tahoe, perhaps a
>bit afterward).  Here is the fix.  Your lines numbers may not match.

Oops, you have to change sys_generic.c as well:

*** /tmp/,RCSt1006180	Sat Sep 23 15:49:38 1989
--- /tmp/,RCSt2006180	Sat Sep 23 15:49:41 1989
***************
*** 136,140 ****
  	}
  	count = uio->uio_resid;
- 	uio->uio_offset = fp->f_offset;
  	if (setjmp(&u.u_qsave)) {
  		if (uio->uio_resid == count) {
--- 137,140 ----
***************
*** 147,151 ****
  		u.u_error = (*fp->f_ops->fo_rw)(fp, rw, uio);
  	u.u_r.r_val1 = count - uio->uio_resid;
- 	fp->f_offset += u.u_r.r_val1;
  }
  
--- 147,150 ----
-- 
In-Real-Life: Chris Torek, Univ of MD Comp Sci Dept (+1 301 454 7163)
Domain:	chris@mimsy.umd.edu	Path:	uunet!mimsy!chris