shithub: riscv

Download patch

ref: 12fa017f3fc2687d3afb166fd30d9b134abb1da5
parent: c45458b929e0f8e5eb26fd97c844977c893f6262
author: cinap_lenrek <[email protected]>
date: Sat Sep 21 12:36:40 EDT 2019

devproc: fix fishy locking in proctext(), check proc validity, static functions

the locking in proctext() is wrong. we have to acquire Proc.seglock
when reading segments from Proc.seg[] as segments do not
have a private freelist and can therefore be reused for other
data structures.

once we have Proc.seglock acquired, check that the process pid
is still valid so we wont accidentally read some other processes
segments. (for both proctext() and procctlmemio()). this also
should give better error message to distinguish the case when
the process did segdetach() the segment in question before we
could acquire Proc.seglock.

declare private functions as static.

--- a/sys/src/9/port/devproc.c
+++ b/sys/src/9/port/devproc.c
@@ -154,11 +154,10 @@
 #define	PID(q)		((q).vers)
 #define	NOTEID(q)	((q).vers)
 
-void	procctlreq(Proc*, char*, int);
-long	procctlmemio(Chan*, Proc*, uintptr, void*, long, int);
-Chan*	proctext(Chan*, Proc*);
-int	procstopped(void*);
-ulong	procpagecount(Proc *);
+static void	procctlreq(Proc*, char*, int);
+static long	procctlmemio(Chan*, Proc*, uintptr, void*, long, int);
+static Chan*	proctext(Chan*, Proc*);
+static int	procstopped(void*);
 
 static Traceevent *tevents;
 static Lock tlock;
@@ -382,10 +381,10 @@
 	case Qtext:
 		if(omode != OREAD)
 			error(Eperm);
-		tc = proctext(c, p);
-		tc->offset = 0;
 		qunlock(&p->debug);
 		poperror();
+		tc = proctext(c, p);
+		tc->offset = 0;
 		cclose(c);
 		return tc;
 
@@ -1297,7 +1296,7 @@
 	procwstat,
 };
 
-Chan*
+static Chan*
 proctext(Chan *c, Proc *p)
 {
 	Chan *tc;
@@ -1304,43 +1303,32 @@
 	Image *i;
 	Segment *s;
 
-	s = p->seg[TSEG];
-	if(s == nil)
-		error(Enonexist);
-	if(p->state==Dead)
+	eqlock(&p->seglock);
+	if(waserror()){
+		qunlock(&p->seglock);
+		nexterror();
+	}
+	if(p->state == Dead || p->pid != PID(c->qid))
 		error(Eprocdied);
-
-	i = s->image;
-	if(i == nil)
-		error(Eprocdied);
-
+	if((s = p->seg[TSEG]) == nil)
+		error(Enonexist);
+	if((i = s->image) == nil)
+		error(Enonexist);
 	lock(i);
-	if(waserror()) {
+	tc = i->c;
+	if(i->notext || tc == nil || (tc->flag&COPEN) == 0 || tc->mode != OREAD){
 		unlock(i);
-		nexterror();
+		error(Enonexist);
 	}
-		
-	tc = i->c;
-	if(tc == nil)
-		error(Eprocdied);
-
-	if(incref(tc) == 1 || (tc->flag&COPEN) == 0 || tc->mode != OREAD) {
-		cclose(tc);
-		error(Eprocdied);
-	}
-
-	if(p->pid != PID(c->qid)) {
-		cclose(tc);
-		error(Eprocdied);
-	}
-
+	incref(tc);
 	unlock(i);
+	qunlock(&p->seglock);
 	poperror();
 
 	return tc;
 }
 
-void
+static void
 procstopwait(Proc *p, int ctl)
 {
 	char *state;
@@ -1375,7 +1363,7 @@
 		error(Eprocdied);
 }
 
-void
+static void
 procctlclosefiles(Proc *p, int all, int fd)
 {
 	Fgrp *f;
@@ -1440,7 +1428,7 @@
 	return nil;
 }
 
-void
+static void
 procctlreq(Proc *p, char *va, int n)
 {
 	Segment *s;
@@ -1636,13 +1624,13 @@
 	free(cb);
 }
 
-int
+static int
 procstopped(void *a)
 {
 	return ((Proc*)a)->state == Stopped;
 }
 
-long
+static long
 procctlmemio(Chan *c, Proc *p, uintptr offset, void *a, long n, int read)
 {
 	Segio *sio;
@@ -1657,11 +1645,9 @@
 		qunlock(&p->seglock);
 		nexterror();
 	}
-	sio = c->aux;
-	if(sio == nil){
-		sio = smalloc(sizeof(Segio));
-		c->aux = sio;
-	}
+	if(p->state == Dead || p->pid != PID(c->qid))
+		error(Eprocdied);
+
 	for(i = 0; i < NSEG; i++) {
 		if(p->seg[i] == s)
 			break;
@@ -1668,6 +1654,7 @@
 	}
 	if(i == NSEG)
 		error(Egreg);	/* segment gone */
+
 	eqlock(s);
 	if(waserror()){
 		qunlock(s);
@@ -1681,6 +1668,13 @@
 	incref(s);		/* for us while we copy */
 	qunlock(s);
 	poperror();
+
+	sio = c->aux;
+	if(sio == nil){
+		sio = smalloc(sizeof(Segio));
+		c->aux = sio;
+	}
+
 	qunlock(&p->seglock);
 	poperror();