shithub: riscv

Info  •  Files  •  Log  •  Branches

ape: fix thread race with close() and select()

in ape close(), do the real filedescriptor _CLOSE() *after* we cleared
the _fdinfo[] slot because once closed, we dont own the slot anymore and
another process doing open() can trash the slot. make sure open() retuns
fd < OPEN_MAX.

double check in _startbuf() holding mux->lock if the fd is already buffered
preveting running double copyprocs on a fd.

dont zero the mux->rwant/ewant bitmaps at the end of select() as we do not
hold the mix->lock.

in _closebuf() kill copyproc while holding the mux->lock to make sure the
copyproc isnt holding it at the time it is killed. run kill() multiple times
to make sure the proc is gone.

--- a/sys/src/ape/lib/ap/plan9/_buf.c

+++ b/sys/src/ape/lib/ap/plan9/_buf.c

@@ -71,10 +71,20 @@

 	lock(&mux->lock);

 	f = &_fdinfo[fd];

+	if((f->flags&FD_ISOPEN) == 0){

+		unlock(&mux->lock);

+		errno = EBADF;

+		return -1;

+	}

 	if((f->flags&FD_BUFFERED) != 0){

 		unlock(&mux->lock);

 		return 0;

 	}

+	if((f->flags&FD_BUFFEREDX) != 0){

+		unlock(&mux->lock);

+		errno = EIO;

+		return -1;

+	}

 	slot = mux->curfds++;

 	if(mux->curfds > INITBUFS) {

@@ -127,14 +137,18 @@

 _closebuf(int fd)

 {

 	Muxbuf *b;

+	int i;

 	b = _fdinfo[fd].buf;

-	if(!b)

+	if(b == 0 || mux == 0)

 		return;

 	lock(&mux->lock);

-	b->fd = -1;

+	if(b->fd == fd){

+		b->fd = -1;

+		for(i=0; i<10 && kill(b->copypid, SIGKILL)==0; i++)

+			_SLEEP(1);

+	}

 	unlock(&mux->lock);

-	kill(b->copypid, SIGKILL);

 }

 /* child copy procs execute this until eof */

@@ -149,7 +163,7 @@

 	for(;;) {

 		/* make sure there's room */

 		lock(&mux->lock);

-		if(e - b->putnext < READMAX) {

+		if(b->fd == fd && (e - b->putnext) < READMAX) {

 			if(b->getnext == b->putnext) {

 				b->getnext = b->putnext = b->data;

 				unlock(&mux->lock);

@@ -168,12 +182,15 @@

 		 */

 		nzeros = 0;

 		do {

+			if(b->fd != fd)

+				break;

 			n = _READ(fd, b->putnext, READMAX);

-			if(b->fd == -1) {

-				_exit(0);		/* we've been closed */

-			}

-		} while(n == 0 && ++nzeros < 3);

+		} while(b->fd == fd && n == 0 && ++nzeros < 3);

 		lock(&mux->lock);

+		if(b->fd != fd){

+			unlock(&mux->lock);

+			_exit(0);	/* we've been closed */

+		}

 		if(n <= 0) {

 			b->eof = 1;

 			if(mux->selwait && FD_ISSET(fd, &mux->ewant)) {

@@ -222,6 +239,11 @@

 	int ngot;

 	b = _fdinfo[fd].buf;

+	if(b == nil || b->fd != fd){

+badfd:

+		errno = EBADF;

+		return -1;

+	}

 	if(b->eof && b->n == 0) {

 goteof:

 		return 0;

@@ -230,8 +252,12 @@

 		errno = EAGAIN;

 		return -1;

 	}

-	/* make sure there's data */

 	lock(&mux->lock);

+	if(b->fd != fd){

+		unlock(&mux->lock);

+		goto badfd;

+	}

+	/* make sure there's data */

 	ngot = b->putnext - b->getnext;

 	if(ngot == 0) {

 		/* maybe EOF just happened */

@@ -244,6 +270,10 @@

 		unlock(&mux->lock);

 		_RENDEZVOUS((unsigned long)&b->datawait, 0);

 		lock(&mux->lock);

+		if(b->fd != fd){

+			unlock(&mux->lock);

+			goto badfd;

+		}

 		ngot = b->putnext - b->getnext;

 	}

 	if(ngot == 0) {

@@ -285,7 +315,8 @@

 		return 0;

 	}

-	_startbuf(-1);

+	if(_startbuf(-1) != 0)

+		return -1;

 	/* make sure all requested rfds and efds are buffered */

 	if(nfds >= OPEN_MAX)

@@ -363,9 +394,10 @@

 	mux->selwait = 1;

 	unlock(&mux->lock);

 	fd = _RENDEZVOUS((unsigned long)&mux->selwait, 0);

-	if(fd >= 0) {

+	if(fd >= 0 && fd < nfds) {

 		b = _fdinfo[fd].buf;

-		if(FD_ISSET(fd, &mux->rwant)) {

+		if(b == 0 || b->fd != fd) {

+		} else  if(FD_ISSET(fd, &mux->rwant)) {

 			FD_SET(fd, rfds);

 			n = 1;

 		} else if(FD_ISSET(fd, &mux->ewant) && b->eof && b->n == 0) {

@@ -373,8 +405,6 @@

 			n = 1;

 		}

 	}

-	FD_ZERO(&mux->rwant);

-	FD_ZERO(&mux->ewant);

 	return n;

 }

--- a/sys/src/ape/lib/ap/plan9/close.c

+++ b/sys/src/ape/lib/ap/plan9/close.c

@@ -20,9 +20,6 @@

 				_closebuf(d);

 			f->flags &= ~FD_BUFFERED;

 		}

-		n = _CLOSE(d);

-		if(n < 0)

-			_syserrno();

 		_fdinfo[d].flags = 0;

 		_fdinfo[d].oflags = 0;

 		if(_fdinfo[d].name){

@@ -29,6 +26,9 @@

 			free(_fdinfo[d].name);

 			_fdinfo[d].name = 0;

 		}

+		n = _CLOSE(d);

+		if(n < 0)

+			_syserrno();

 	}

 	return n;

 }

--- a/sys/src/ape/lib/ap/plan9/open.c

+++ b/sys/src/ape/lib/ap/plan9/open.c

@@ -46,6 +46,11 @@

 		if(n < 0)

 			_syserrno();

 	}

+	if(n >= OPEN_MAX){

+		_CLOSE(n);

+		errno = ENFILE;

+		return -1;

+	}

 	if(n >= 0){

 		fi = &_fdinfo[n];

 		fi->flags = FD_ISOPEN;

--- a/sys/src/ape/lib/ap/plan9/read.c

+++ b/sys/src/ape/lib/ap/plan9/read.c

@@ -38,7 +38,7 @@

 		}

 		n = _readbuf(d, buf, nbytes, noblock);

 	}else{

-		n = _READ(d,  buf, nbytes);

+		n = _READ(d, buf, nbytes);

 		if(n < 0)

 			_syserrno();

 	}