Re: [9fans] Potential race in syspipe()

[presotto@plan9.bell-labs.com]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

If the user is doing a dup2 unsynchronized with the syspipe(),
even if everything works, the results are wierd with both solutions.
Both the pipe() and the dup2() can succeed, except the pipe()
can be returning fd's that have nothing to do with a pipe.
As you said, dup2 is an dangerous thing when the fd's are shared.

However, you're right.  Your solution yields a more explainable
world in the unlikely event that newfd() fails in syspipe().
I put in a newfd2().  The exact code follows.  I added the
routine findfreefd() since I want to keep that piece of
magic in one place.  I noticed your snippet didn't check the
need to growfd after getting i, perhaps a good reason for
findfreefd().

/*
 *  this assumes that the fgrp is locked
 */
int
findfreefd(Fgrp *f, int start)
{
	int fd;

	for(fd=start; fd<f->nfd; fd++)
		if(f->fd[fd] == 0)
			break;
	if(fd >= f->nfd && growfd(f, fd) < 0)
		return -1;
	return fd;
}

int
newfd(Chan *c)
{
	int fd;
	Fgrp *f;

	f = up->fgrp;
	lock(f);
	fd = findfreefd(f, 0);
	if(fd < 0){
		unlock(f);
		return -1;
	}
	if(fd > f->maxfd)
		f->maxfd = fd;
	f->fd[fd] = c;
	unlock(f);
	return fd;
}

int
newfd2(int fd[2], Chan *c[2])
{
	Fgrp *f;

	f = up->fgrp;
	lock(f);
	fd[0] = findfreefd(f, 0);
	if(fd[0] < 0){
		unlock(f);
		return -1;
	}
	fd[1] = findfreefd(f, fd[0]+1);
	if(fd[1] < 0){
		unlock(f);
		return -1;
	}
	if(fd[1] > f->maxfd)
		f->maxfd = fd[1];
	f->fd[fd[0]] = c[0];
	f->fd[fd[1]] = c[1];
	unlock(f);

	return 0;
}

...

long
syspipe(ulong *arg)
{
	int fd[2];
	Chan *c[2];
	Dev *d;

	validaddr(arg[0], 2*BY2WD, 1);
	evenaddr(arg[0]);
	d = devtab[devno('|', 0)];
	c[0] = namec("#|", Atodir, 0, 0);
	c[1] = 0;
	fd[0] = -1;
	fd[1] = -1;

	if(waserror()){
		cclose(c[0]);
		if(c[1])
			cclose(c[1]);
		nexterror();
	}
	c[1] = cclone(c[0], 0);
	if(walk(&c[0], "data", 1) < 0)
		error(Egreg);
	if(walk(&c[1], "data1", 1) < 0)
		error(Egreg);
	c[0] = d->open(c[0], ORDWR);
	c[1] = d->open(c[1], ORDWR);
	if(newfd2(fd, c) < 0)
		error(Enofd);
	poperror();

	((long*)arg[0])[0] = fd[0];
	((long*)arg[0])[1] = fd[1];
	return 0;
}

As for create(), I'ld rather change the man page.  Otherwise we'ld
have to reserve an fd in some way before the open but not have that
fd visible to other processes until it was assinged a channel
or let it be step on-able by dup2...

[-- Attachment #2: Type: message/rfc822, Size: 4041 bytes --]

From: Alexander Viro <viro@math.psu.edu>
To: 9fans@cse.psu.edu
Subject: Re: [9fans] Potential race in syspipe()
Date: Sun, 13 Aug 2000 22:42:05 -0400 (EDT)
Message-ID: <Pine.GSO.4.10.10008132154210.194-100000@weyl.math.psu.edu>



On Sun, 13 Aug 2000 presotto@plan9.bell-labs.com wrote:

> You are right, it is rookie programming, probably mine.  Your statement:
> 
> 	_anything_ that goes into ->fd[] must be treated as final
> 	as soon as you release the lock on ->fgrp
> 
> is the crux of the matter.
> 
> Your solution is reasonable.  I would probably do something a little
> dirtier to avoid duplicating the functionality of newfd().  It would be
> just to make the waserror() a little smarter in syspipe():
> 
> 	if(waserror()){
> 		if(fd[0] >= 0)
> 			fdclose(fd[0])
> 		else
> 			cclose(c[0]);
> 		if(fd[1] >= 0)
> 			fdclose(fd[1]);
> 		else if(c[1] != nil)
> 			cclose(c[1]);
> 		nexterror();
> 	}
> 
> and leave the rest alone.

Umm... You are handling that race to userland - effect of the race
scenario will be
Process A: pipe() returns -1
Process B: dup(foo, bar) returns bar, but fd[bar]==NULL

OTOH the race _was_ in the userland from the very beginning, so it may be
OK. Sigh... dup2() semantics seems to be ill-suited for systems that can
share descriptor table between several processes. Probably Plan 9 was the
first system that had to deal with that (IIRC Linux got clone() in '94 or
'95 and *BSD got rfork() pretty recently). Were there any attempts to
provide something, erm, less race-prone than dup2()?

	BTW, looks like create(2) (and creat() in APE) have an interesting
misfeature/bug/whatever: suppose that file creation succeeds but newfd()
fails. Then you are getting Enofd, but file is already created (and stays
alive, indeed). For APE it's a bug - directly violates POSIX. For the
native code... Hell knows, but it seems to be at least worth mentioning in
the manpage.

	I realize very well that changing that would be painful - the best
strategy is probably to ask for space at the very beginning and keep the
number of pending allocations (i.e. make sure that newfd() never fails).
Yuck. I'll probably try to see what this approach gives on our side, but
I'm not sure that it will be any better than our current code (right now
we are using an equivalent of falloc(9); since we keep a bitmap of
allocated descriptors we can see if dup2() tries to stick its nose into
the pending slot). newfd() equivalent had been tried some time ago, but
back then it was rejected due to problems with creat(2).
	At least you don't have to deal with bloody SCM_RIGHTS
file-passing...

								Al