From mboxrd@z Thu Jan  1 00:00:00 1970
Date: Fri, 11 Aug 2000 07:20:14 -0400
From: Alexander Viro <viro@math.psu.edu>
To: 9fans@cse.psu.edu
Subject: [9fans] Potential race in syspipe()
Message-ID: <Pine.GSO.4.10.10008110640100.194-100000@weyl.math.psu.edu>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Topicbox-Message-UUID: f9c79ac8-eac8-11e9-9e20-41e7f4b1d025


	Umm... Unless I'm seriously misreading the code, there is a race
between syspipe() and sysdup(). Scenario:

CPU1 (in syspipe()):
        c[0] = d->open(c[0], ORDWR);
        c[1] = d->open(c[1], ORDWR);
        fd[0] = newfd(c[0]);
/* got so far */
        fd[1] = newfd(c[1]);
/* failed on that one for whatever reason */

CPU2 (in sysdup()):
		...
                lock(f);
		...
                oc = f->fd[fd];
                f->fd[fd] = c;
                unlock(f);
                if(oc)
                        cclose(oc);
/* where fd happens to be equal to fd[0] on the CPU1 */
/* oc == CPU1's c[0], indeed */

CPU1:
        if(fd[0] < 0 || fd[1] < 0)
                error(Enofd);
/* there we go */
                cclose(c[0]);
/* woops - extra cclose on c[0] and we are welcome to... */
        if(c->flag&CFREE)
                panic("cclose %lux", getcallerpc(&c));


Giving user enough rope to hang himself is a good idea, but panic() goes
somewhat beyond "himself", especially when triggered on CPU server...
AFAICS the cleanish solution would be along the lines of

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

        f = up->fgrp;
        lock(f);
        for(i=0; i<f->nfd; i++)
                if(f->fd[i] == 0)
                        break;
	for(j=i+1; j<f->nfd; j++)
		if (f->fd[j] == 0)
			break;
      
        if(j >= f->nfd && growfd(f, j) < 0){
                unlock(f);
                return -1;
        }
        if(j > f->maxfd)
                f->maxfd = j;
        f->fd[fd[0]=i] = c[0];
        f->fd[fd[1]=j] = c[1];
        unlock(f);
        return 0;
}

and using it in syspipe() (instead of two newfd() calls). That way the
error recovery in syspipe() doesn't have to touch anything that might be
anywhere near the ->fd[].

Comments? AFAICS _anything_ that goes into ->fd[] must be treated as final
as soon as you release the lock on ->fgrp (due to the dup2() issues).

							Al