[9front] OCEXEC and ORCLOSE issues and solutions

[9front] OCEXEC and ORCLOSE issues and solutions

[cinap_lenrek]

There are some tricky corner cases with ORCLOSE and OCEXEC
open(2) flags with regard to the channel passing
drivers like devsrv (/srv), devshr (/shr) and devdup (/fd).

There are two issues:

1)

Because OCEXEC flag is *shared* between all file descriptors
pointing to same chan, it is possible to set the close-on-exec
flag on a file posted to /srv. It is also possible to set
close-on-exec on the file that you inherited from a parent
process. So it would be closed in the parent as well if it
would do a exec.

2)

It was possible to retroactively set the ORCLOSE flag on
a file descriptor or file posted to /srv and /fd.

I'v currently running a kernel that is modified in the following way:

Problem 1)

The OCEXEC flag semantics have been changed to always refer
to the *file descriptor* instead of the channel. (basically
the same as posix FD_CLOEXEC).

So when opening a file with OCEXEC (even /fd/N),
the close-on-exec applies only to the file descriptor that
open returned, not anyone else.

When the file descriptors are copied with rfork(RFFDG),
the per file descriptor flags are also copied.

The dup(2) syscall always resets the OCEXEC flag for newfd.

Note that if you want to have it close on exec, you can
just reopen it with /fd.

Problem 2)

The remove-on-close flag makes sense to be shared by all
the file descriptors. We just need to make sure to not set
it retroactively in the relevant drivers.

The solution is to have devfd, devsrv and devshr error out
of open with Eperm[] when ORCLOSE flag was given.

A patch of these modification follows. 

Please make some noise if there are any objections to
these changes or bugs spotted.

diff -r dcd57f411618 sys/src/9/port/auth.c
--- a/sys/src/9/port/auth.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/auth.c	Wed Dec 09 17:37:26 2020 +0100
@@ -97,13 +97,12 @@
 		nexterror();
 	}
 
-	fd = newfd(ac);
+	/* always mark it close on exec */
+	fd = newfd(ac, OCEXEC);
 	if(fd < 0)
 		error(Enofd);
 	poperror();	/* ac */
 
-	/* always mark it close on exec */
-	ac->flag |= CCEXEC;
 	return (uintptr)fd;
 }
 
diff -r dcd57f411618 sys/src/9/port/chan.c
--- a/sys/src/9/port/chan.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/chan.c	Wed Dec 09 17:37:26 2020 +0100
@@ -1468,9 +1468,6 @@
 			saveregisters();
 
 			c = devtab[c->type]->open(c, omode&~OCEXEC);
-
-			if(omode & OCEXEC)
-				c->flag |= CCEXEC;
 			if(omode & ORCLOSE)
 				c->flag |= CRCLOSE;
 			break;
@@ -1571,11 +1568,9 @@
 			incref(cnew->path);
 
 			cnew = devtab[cnew->type]->create(cnew, e.elems[e.nelems-1], omode&~(OEXCL|OCEXEC), perm);
-			poperror();
-			if(omode & OCEXEC)
-				cnew->flag |= CCEXEC;
 			if(omode & ORCLOSE)
 				cnew->flag |= CRCLOSE;
+			poperror();
 			putmhead(m);
 			cclose(c);
 			c = cnew;
diff -r dcd57f411618 sys/src/9/port/devdup.c
--- a/sys/src/9/port/devdup.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/devdup.c	Wed Dec 09 17:37:26 2020 +0100
@@ -63,6 +63,8 @@
 	Chan *f;
 	int fd, twicefd;
 
+	if(omode & ORCLOSE)
+		error(Eperm);
 	if(c->qid.type & QTDIR){
 		if(omode != 0)
 			error(Eisdir);
diff -r dcd57f411618 sys/src/9/port/devshr.c
--- a/sys/src/9/port/devshr.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/devshr.c	Wed Dec 09 17:37:26 2020 +0100
@@ -396,6 +396,8 @@
 	case Qcmpt:
 		if(omode&OTRUNC)
 			error(Eexist);
+		if(omode&ORCLOSE)
+			error(Eperm);
 		shr = sch->shr;
 		mpt = sch->mpt;
 		devpermcheck(mpt->owner, mpt->perm, mode);
diff -r dcd57f411618 sys/src/9/port/devsrv.c
--- a/sys/src/9/port/devsrv.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/devsrv.c	Wed Dec 09 17:37:26 2020 +0100
@@ -135,6 +135,8 @@
 
 	if(omode&OTRUNC)
 		error(Eexist);
+	if(omode&ORCLOSE)
+		error(Eperm);
 	if(openmode(omode)!=sp->chan->mode && sp->chan->mode!=ORDWR)
 		error(Eperm);
 	devpermcheck(sp->owner, sp->perm, omode);
@@ -338,8 +340,6 @@
 		cclose(c1);
 		nexterror();
 	}
-	if(c1->flag & (CCEXEC|CRCLOSE))
-		error("posted fd has remove-on-close or close-on-exec");
 	if(c1->qid.type & QTAUTH)
 		error("cannot post auth file in srv");
 	sp = srvlookup(nil, c->qid.path);
diff -r dcd57f411618 sys/src/9/port/lib.h
--- a/sys/src/9/port/lib.h	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/lib.h	Wed Dec 09 17:37:26 2020 +0100
@@ -176,7 +176,7 @@
 #define	ORDWR	2	/* read and write */
 #define	OEXEC	3	/* execute, == read but check execute permission */
 #define	OTRUNC	16	/* or'ed in (except for exec), truncate file first */
-#define	OCEXEC	32	/* or'ed in, close on exec */
+#define	OCEXEC	32	/* or'ed in (per file descriptor), close on exec */
 #define	ORCLOSE	64	/* or'ed in, remove on close */
 #define OEXCL   0x1000	/* or'ed in, exclusive create */
 
diff -r dcd57f411618 sys/src/9/port/pgrp.c
--- a/sys/src/9/port/pgrp.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/pgrp.c	Wed Dec 09 17:37:26 2020 +0100
@@ -140,7 +140,8 @@
 
 	new = smalloc(sizeof(Fgrp));
 	if(f == nil){
-		new->fd = smalloc(DELTAFD*sizeof(Chan*));
+		new->flag = smalloc(DELTAFD*sizeof(new->flag[0]));
+		new->fd = smalloc(DELTAFD*sizeof(new->fd[0]));
 		new->nfd = DELTAFD;
 		new->ref = 1;
 		return new;
@@ -152,12 +153,19 @@
 	i = new->nfd%DELTAFD;
 	if(i != 0)
 		new->nfd += DELTAFD - i;
-	new->fd = malloc(new->nfd*sizeof(Chan*));
+	new->fd = malloc(new->nfd*sizeof(new->fd[0]));
 	if(new->fd == nil){
 		unlock(f);
 		free(new);
 		error("no memory for fgrp");
 	}
+	new->flag = malloc(new->nfd*sizeof(new->flag[0]));
+	if(new->flag == nil){
+		unlock(f);
+		free(new->fd);
+		free(new);
+		error("no memory for fgrp");
+	}
 	new->ref = 1;
 
 	new->maxfd = f->maxfd;
@@ -165,6 +173,7 @@
 		if((c = f->fd[i]) != nil){
 			incref(c);
 			new->fd[i] = c;
+			new->flag[i] = f->flag[i];
 		}
 	}
 	unlock(f);
@@ -194,6 +203,7 @@
 	up->closingfgrp = nil;
 
 	free(f->fd);
+	free(f->flag);
 	free(f);
 }
 
diff -r dcd57f411618 sys/src/9/port/portdat.h
--- a/sys/src/9/port/portdat.h	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/portdat.h	Wed Dec 09 17:37:26 2020 +0100
@@ -125,7 +125,7 @@
 	COPEN	= 0x0001,		/* for i/o */
 	CMSG	= 0x0002,		/* the message channel for a mount */
 /*rsc	CCREATE	= 0x0004,		/* permits creation if c->mnt */
-	CCEXEC	= 0x0008,		/* close on exec */
+	CCEXEC	= 0x0008,		/* close on exec (per file descriptor) */
 	CFREE	= 0x0010,		/* not in use */
 	CRCLOSE	= 0x0020,		/* remove on close */
 	CCACHE	= 0x0080,		/* client cache */
@@ -509,6 +509,7 @@
 	Ref;
 	Lock;
 	Chan	**fd;
+	uchar	*flag;			/* per file-descriptor flags (CCEXEC) */
 	int	nfd;			/* number allocated */
 	int	maxfd;			/* highest fd in use */
 	int	exceed;			/* debugging */
diff -r dcd57f411618 sys/src/9/port/portfns.h
--- a/sys/src/9/port/portfns.h	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/portfns.h	Wed Dec 09 17:37:26 2020 +0100
@@ -201,7 +201,7 @@
 void		nameerror(char*, char*);
 int		needpages(void*);
 Chan*		newchan(void);
-int		newfd(Chan*);
+int		newfd(Chan*, int);
 Mhead*		newmhead(Chan*);
 Mount*		newmount(Chan*, int, char*);
 Page*		newpage(int, Segment **, uintptr);
diff -r dcd57f411618 sys/src/9/port/sysfile.c
--- a/sys/src/9/port/sysfile.c	Wed Dec 09 01:05:14 2020 +0100
+++ b/sys/src/9/port/sysfile.c	Wed Dec 09 17:37:26 2020 +0100
@@ -25,33 +25,45 @@
 growfd(Fgrp *f, int fd)	/* fd is always >= 0 */
 {
 	Chan **newfd, **oldfd;
+	uchar *newflag, *oldflag;
+	int nfd;
 
-	if(fd < f->nfd)
+	nfd = f->nfd;
+	if(fd < nfd)
 		return 0;
-	if(fd >= f->nfd+DELTAFD)
+	if(fd >= nfd+DELTAFD)
 		return -1;	/* out of range */
 	/*
 	 * Unbounded allocation is unwise; besides, there are only 16 bits
 	 * of fid in 9P
 	 */
-	if(f->nfd >= 5000){
+	if(nfd >= 5000){
     Exhausted:
 		print("no free file descriptors\n");
 		return -1;
 	}
-	newfd = malloc((f->nfd+DELTAFD)*sizeof(Chan*));
+	oldfd = f->fd;
+	oldflag = f->flag;
+	newfd = malloc((nfd+DELTAFD)*sizeof(newfd[0]));
 	if(newfd == nil)
 		goto Exhausted;
-	oldfd = f->fd;
-	memmove(newfd, oldfd, f->nfd*sizeof(Chan*));
+	memmove(newfd, oldfd, nfd*sizeof(newfd[0]));
+	newflag = malloc((nfd+DELTAFD)*sizeof(newflag[0]));
+	if(newfd == nil){
+		free(newfd);
+		goto Exhausted;
+	}
+	memmove(newflag, oldflag, nfd*sizeof(newflag[0]));
 	f->fd = newfd;
-	free(oldfd);
-	f->nfd += DELTAFD;
+	f->flag = newflag;
+	f->nfd = nfd+DELTAFD;
 	if(fd > f->maxfd){
 		if(fd/100 > f->maxfd/100)
 			f->exceed = (fd/100)*100;
 		f->maxfd = fd;
 	}
+	free(oldfd);
+	free(oldflag);
 	return 1;
 }
 
@@ -72,9 +84,9 @@
 }
 
 int
-newfd(Chan *c)
+newfd(Chan *c, int mode)
 {
-	int fd;
+	int fd, flag;
 	Fgrp *f;
 
 	f = up->fgrp;
@@ -87,6 +99,13 @@
 	if(fd > f->maxfd)
 		f->maxfd = fd;
 	f->fd[fd] = c;
+
+	/* per file-descriptor flags */
+	flag = 0;
+	if(mode & OCEXEC)
+		flag |= CCEXEC;
+	f->flag[fd] = flag;
+
 	unlockfgrp(f);
 	return fd;
 }
@@ -112,6 +131,8 @@
 		f->maxfd = fd[1];
 	f->fd[fd[0]] = c[0];
 	f->fd[fd[1]] = c[1];
+	f->flag[fd[0]] = 0;
+	f->flag[fd[1]] = 0;
 	unlockfgrp(f);
 	return 0;
 }
@@ -247,6 +268,7 @@
 
 		oc = f->fd[fd];
 		f->fd[fd] = c;
+		f->flag[fd] = 0;
 		unlockfgrp(f);
 		if(oc != nil)
 			cclose(oc);
@@ -255,7 +277,7 @@
 			cclose(c);
 			nexterror();
 		}
-		fd = newfd(c);
+		fd = newfd(c, 0);
 		if(fd < 0)
 			error(Enofd);
 		poperror();
@@ -280,7 +302,7 @@
 		cclose(c);
 		nexterror();
 	}
-	fd = newfd(c);
+	fd = newfd(c, mode);
 	if(fd < 0)
 		error(Enofd);
 	poperror();
@@ -295,7 +317,7 @@
 
 	lock(f);
 	c = fd <= f->maxfd ? f->fd[fd] : nil;
-	if(c == nil || (flag != 0 && (c->flag&flag) == 0)){
+	if(c == nil || (flag != 0 && ((f->flag[fd]|c->flag)&flag) == 0)){
 		unlock(f);
 		return;
 	}
@@ -1166,7 +1188,7 @@
 		cclose(c);
 		nexterror();
 	}
-	fd = newfd(c);
+	fd = newfd(c, mode);
 	if(fd < 0)
 		error(Enofd);
 	poperror();