From mboxrd@z Thu Jan  1 00:00:00 1970
From: presotto@plan9.bell-labs.com
Message-Id: <200008140147.VAA29002@cse.psu.edu>
Date: Sun, 13 Aug 2000 21:47:28 -0400
To: 9fans@cse.psu.edu
Subject: Re: [9fans] Potential race in syspipe()
MIME-Version: 1.0
Content-Type: multipart/mixed;
	boundary="upas-ktdpugqymxtskmpqzdrdgibmvi"
Topicbox-Message-UUID: facfa08c-eac8-11e9-9e20-41e7f4b1d025

This is a multi-part message in MIME format.
--upas-ktdpugqymxtskmpqzdrdgibmvi
Content-Disposition: inline
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

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.

--upas-ktdpugqymxtskmpqzdrdgibmvi
Content-Type: message/rfc822
Content-Disposition: inline

Received: from plan9.cs.bell-labs.com ([135.104.9.2]) by plan9; Fri Aug 11 07:38:04 EDT 2000
Received: from cse.psu.edu ([130.203.3.50]) by plan9; Fri Aug 11 07:38:02 EDT 2000
Received: from localhost (majordom@localhost)
	by cse.psu.edu (8.8.8/8.8.8) with SMTP id HAA09476;
	Fri, 11 Aug 2000 07:22:44 -0400 (EDT)
Received: by claven.cse.psu.edu (bulk_mailer v1.5); Fri, 11 Aug 2000 07:20:29 -0400
Received: (from majordom@localhost)
	by cse.psu.edu (8.8.8/8.8.8) id HAA09383
	for 9fans-outgoing; Fri, 11 Aug 2000 07:20:23 -0400 (EDT)
Received: from math.psu.edu (leibniz.math.psu.edu [146.186.130.2])
	by cse.psu.edu (8.8.8/8.8.8) with ESMTP id HAA09379
	for <9fans@cse.psu.edu>; Fri, 11 Aug 2000 07:20:15 -0400 (EDT)
Received: from weyl.math.psu.edu (weyl.math.psu.edu [146.186.130.226])
	by math.psu.edu (8.9.3/8.9.3) with ESMTP id HAA15869
	for <9fans@cse.psu.edu>; Fri, 11 Aug 2000 07:20:15 -0400 (EDT)
Received: from localhost (viro@localhost)
	by weyl.math.psu.edu (8.9.3/8.9.3) with ESMTP id HAA00462
	for <9fans@cse.psu.edu>; Fri, 11 Aug 2000 07:20:14 -0400 (EDT)
X-Authentication-Warning: weyl.math.psu.edu: viro owned process doing -bs
Date: Fri, 11 Aug 2000 07:20:14 -0400 (EDT)
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
Sender: owner-9fans@cse.psu.edu
Reply-To: 9fans@cse.psu.edu
Precedence: bulk


	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

--upas-ktdpugqymxtskmpqzdrdgibmvi--