changelog shortlog tags branches files raw gz bz2 help

Mercurial > hg > plan9front / changeset: sshfs: fix race condition between sendproc() and recvproc()

changeset 7409: 9291b5e14042
parent 7408: 1d345066125a
child 7410: e9634a89bcd9
author: cinap_lenrek@felloff.net
date: Mon, 07 Oct 2019 11:52:14 +0200
files: sys/src/cmd/sshfs.c
description: sshfs: fix race condition between sendproc() and recvproc()

there was a race between the sendproc putting the request on
the sreqrd[] id and the recvproc handling the response, and
potentially freeing the request before the sendproc() was
finished with the request (or the fid).

so we defer allocating a request id and putting it on the
sreqrd[] stage after we have completely generated the
request in vpack(). this prevents the handling of the request
before it is even sent.

this also means that the SReq should not be touched after
calling sendpkt(), submitreq(), submitsreq().

secondly, putsfid() needs to acquire the RWLock to make sure
sendproc() is finished with the request. the scenario is that
recvproc() can call respond() on the request before sendproc()
has unlocked the SFid.
     1.1--- a/sys/src/cmd/sshfs.c
     1.2+++ b/sys/src/cmd/sshfs.c
     1.3@@ -103,7 +103,6 @@ struct SFid {
     1.4 struct SReq {
     1.5 	Req *req;
     1.6 	SFid *closefid;
     1.7-	int reqid;
     1.8 	SReq *next;
     1.9 };
    1.10 
    1.11@@ -194,9 +193,29 @@ namelookup(IDEnt **tab, char *name)
    1.12 }
    1.13 
    1.14 int
    1.15+allocsreqid(SReq *r)
    1.16+{
    1.17+	int i;
    1.18+
    1.19+	qlock(&sreqidlock);
    1.20+	for(;;){
    1.21+		for(i = 0; i < MAXREQID; i++)
    1.22+			if(sreqrd[i] == nil){
    1.23+				sreqrd[i] = r;
    1.24+				goto out;
    1.25+			}
    1.26+		rsleep(&sreqidrend);
    1.27+	}
    1.28+out:
    1.29+	qunlock(&sreqidlock);
    1.30+	return i;
    1.31+}
    1.32+
    1.33+int
    1.34 vpack(uchar *p, int n, char *fmt, va_list a)
    1.35 {
    1.36 	uchar *p0 = p, *e = p+n;
    1.37+	SReq *sr = nil;
    1.38 	u32int u;
    1.39 	u64int v;
    1.40 	void *s;
    1.41@@ -205,6 +224,10 @@ vpack(uchar *p, int n, char *fmt, va_lis
    1.42 	for(;;){
    1.43 		switch(c = *fmt++){
    1.44 		case '\0':
    1.45+			if(sr != nil){
    1.46+				u = allocsreqid(sr);
    1.47+				PUT4(p0+1, u);
    1.48+			}
    1.49 			return p - p0;
    1.50 		case '_':
    1.51 			if(++p > e) goto err;
    1.52@@ -228,6 +251,11 @@ vpack(uchar *p, int n, char *fmt, va_lis
    1.53 			memmove(p, s, u);
    1.54 			p += u;
    1.55 			break;
    1.56+		case 'q':
    1.57+			p += 4;
    1.58+			if(p != p0+5 || p > e) goto err;
    1.59+			sr = va_arg(a, SReq*);
    1.60+			break;
    1.61 		case 'u':
    1.62 			u = va_arg(a, int);
    1.63 			if(p+4 > e) goto err;
    1.64@@ -389,11 +417,14 @@ freedir(SFid *s)
    1.65 	s->dirpos = 0;
    1.66 }
    1.67 
    1.68-
    1.69 void
    1.70 putsfid(SFid *s)
    1.71 {
    1.72 	if(s == nil) return;
    1.73+
    1.74+	wlock(s);
    1.75+	wunlock(s);
    1.76+
    1.77 	free(s->fn);
    1.78 	free(s->hand);
    1.79 	freedir(s);
    1.80@@ -403,14 +434,6 @@ putsfid(SFid *s)
    1.81 void
    1.82 putsreq(SReq *s)
    1.83 {
    1.84-	if(s == nil) return;
    1.85-	if(s->reqid != -1){
    1.86-		qlock(&sreqidlock);
    1.87-		sreqrd[s->reqid] = nil;
    1.88-		rwakeup(&sreqidrend);
    1.89-		qunlock(&sreqidlock);
    1.90-	}
    1.91-	putsfid(s->closefid);
    1.92 	free(s);
    1.93 }
    1.94 
    1.95@@ -424,14 +447,12 @@ submitsreq(SReq *s)
    1.96 	qunlock(&sreqwrlock);
    1.97 }
    1.98 
    1.99-
   1.100 void
   1.101 submitreq(Req *r)
   1.102 {
   1.103 	SReq *s;
   1.104 	
   1.105 	s = emalloc9p(sizeof(SReq));
   1.106-	s->reqid = -1;
   1.107 	s->req = r;
   1.108 	submitsreq(s);
   1.109 }
   1.110@@ -697,20 +718,18 @@ readprocess(Req *r)
   1.111 void
   1.112 sshfsread(Req *r)
   1.113 {
   1.114-	SFid *sf;
   1.115-
   1.116 	if((r->fid->qid.type & QTDIR) == 0){
   1.117 		submitreq(r);
   1.118 		return;
   1.119 	}
   1.120-	sf = r->fid->aux;	
   1.121 	if(r->ifcall.offset == 0){
   1.122+		SFid *sf = r->fid->aux;
   1.123 		wlock(sf);
   1.124 		freedir(sf);
   1.125 		if(sf->dirreads > 0){
   1.126+			wunlock(sf);
   1.127 			r->aux = (void*)-1;
   1.128 			submitreq(r);
   1.129-			wunlock(sf);
   1.130 			return;
   1.131 		}
   1.132 		wunlock(sf);
   1.133@@ -764,7 +783,6 @@ sendproc(void *)
   1.134 {
   1.135 	SReq *r;
   1.136 	SFid *sf;
   1.137-	int i;
   1.138 	int x, y;
   1.139 	char *s, *t;
   1.140 
   1.141@@ -778,41 +796,33 @@ sendproc(void *)
   1.142 		sreqwr = r->next;
   1.143 		if(sreqwr == nil) sreqlast = &sreqwr;
   1.144 		qunlock(&sreqwrlock);
   1.145-		
   1.146-		qlock(&sreqidlock);
   1.147-	idagain:
   1.148-		for(i = 0; i < MAXREQID; i++)
   1.149-			if(sreqrd[i] == nil){
   1.150-				sreqrd[i] = r;
   1.151-				r->reqid = i;
   1.152-				break;
   1.153-			}
   1.154-		if(i == MAXREQID){
   1.155-			rsleep(&sreqidrend);
   1.156-			goto idagain;
   1.157-		}
   1.158-		qunlock(&sreqidlock);
   1.159 
   1.160-		if(r->closefid != nil){
   1.161-			sendpkt("bus", SSH_FXP_CLOSE, r->reqid, r->closefid->hand, r->closefid->handn);
   1.162+		sf = r->closefid;
   1.163+		if(sf != nil){
   1.164+			rlock(sf);
   1.165+			sendpkt("bqs", SSH_FXP_CLOSE, r, sf->hand, sf->handn);
   1.166+			runlock(sf);
   1.167 			continue;
   1.168 		}
   1.169 		if(r->req == nil)
   1.170 			sysfatal("nil request in queue");
   1.171 
   1.172-		sf = r->req->fid != nil ? r->req->fid->aux : nil;
   1.173+		sf = r->req->fid->aux;
   1.174 		switch(r->req->ifcall.type){
   1.175 		case Tattach:
   1.176-			sendpkt("bus", SSH_FXP_STAT, r->reqid, sf->fn, strlen(sf->fn));
   1.177+			rlock(sf);
   1.178+			sendpkt("bqs", SSH_FXP_STAT, r, sf->fn, strlen(sf->fn));
   1.179+			runlock(sf);
   1.180 			break;
   1.181 		case Twalk:
   1.182-			sendpkt("bus", SSH_FXP_STAT, r->reqid, r->req->aux, strlen(r->req->aux));
   1.183+			sendpkt("bqs", SSH_FXP_STAT, r, r->req->aux, strlen(r->req->aux));
   1.184 			break;
   1.185 		case Topen:
   1.186-			rlock(sf);
   1.187-			if((r->req->ofcall.qid.type & QTDIR) != 0)
   1.188-				sendpkt("bus", SSH_FXP_OPENDIR, r->reqid, sf->fn, strlen(sf->fn));
   1.189-			else{
   1.190+			if((r->req->ofcall.qid.type & QTDIR) != 0){
   1.191+				rlock(sf);
   1.192+				sendpkt("bqs", SSH_FXP_OPENDIR, r, sf->fn, strlen(sf->fn));
   1.193+				runlock(sf);
   1.194+			}else{
   1.195 				x = r->req->ifcall.mode;
   1.196 				y = 0;
   1.197 				switch(x & 3){
   1.198@@ -822,15 +832,15 @@ sendproc(void *)
   1.199 				}
   1.200 				if(readonly && (y & SSH_FXF_WRITE) != 0){
   1.201 					respond(r->req, "mounted read-only");
   1.202-					runlock(sf);
   1.203 					putsreq(r);
   1.204 					break;
   1.205 				}
   1.206 				if((x & OTRUNC) != 0)
   1.207 					y |= SSH_FXF_TRUNC;
   1.208-				sendpkt("busuu", SSH_FXP_OPEN, r->reqid, sf->fn, strlen(sf->fn), y, 0);
   1.209+				rlock(sf);
   1.210+				sendpkt("bqsuu", SSH_FXP_OPEN, r, sf->fn, strlen(sf->fn), y, 0);
   1.211+				runlock(sf);
   1.212 			}
   1.213-			runlock(sf);
   1.214 			break;
   1.215 		case Tcreate:
   1.216 			rlock(sf);
   1.217@@ -838,12 +848,12 @@ sendproc(void *)
   1.218 			runlock(sf);
   1.219 			if((r->req->ifcall.perm & DMDIR) != 0){
   1.220 				if(r->req->aux == nil){
   1.221-					sendpkt("busuu", SSH_FXP_MKDIR, r->reqid, s, strlen(s),
   1.222+					r->req->aux = (void*)-1;
   1.223+					sendpkt("bqsuu", SSH_FXP_MKDIR, r, s, strlen(s),
   1.224 						SSH_FILEXFER_ATTR_PERMISSIONS, r->req->ifcall.perm & 0777);
   1.225-					r->req->aux = (void*)-1;
   1.226 				}else{
   1.227-					sendpkt("bus", SSH_FXP_OPENDIR, r->reqid, s, strlen(s));
   1.228 					r->req->aux = (void*)-2;
   1.229+					sendpkt("bqs", SSH_FXP_OPENDIR, r, s, strlen(s));
   1.230 				}
   1.231 				free(s);
   1.232 				break;
   1.233@@ -855,7 +865,7 @@ sendproc(void *)
   1.234 			case OWRITE: y |= SSH_FXF_WRITE; break;
   1.235 			case ORDWR: y |= SSH_FXF_READ | SSH_FXF_WRITE; break;
   1.236 			}
   1.237-			sendpkt("busuuu", SSH_FXP_OPEN, r->reqid, s, strlen(s), y,
   1.238+			sendpkt("bqsuuu", SSH_FXP_OPEN, r, s, strlen(s), y,
   1.239 				SSH_FILEXFER_ATTR_PERMISSIONS, r->req->ifcall.perm & 0777);
   1.240 			free(s);
   1.241 			break;
   1.242@@ -863,22 +873,22 @@ sendproc(void *)
   1.243 			if((r->req->fid->qid.type & QTDIR) != 0){
   1.244 				wlock(sf);
   1.245 				if(r->req->aux == (void*)-1){
   1.246-					sendpkt("bus", SSH_FXP_CLOSE, r->reqid, sf->hand, sf->handn);
   1.247+					sendpkt("bqs", SSH_FXP_CLOSE, r, sf->hand, sf->handn);
   1.248 					free(sf->hand);
   1.249 					sf->hand = nil;
   1.250 					sf->handn = 0;
   1.251 					sf->direof = 0;
   1.252 					sf->dirreads = 0;
   1.253 				}else if(r->req->aux == (void*)-2){
   1.254-					sendpkt("bus", SSH_FXP_OPENDIR, r->reqid, sf->fn, strlen(sf->fn));
   1.255+					sendpkt("bqs", SSH_FXP_OPENDIR, r, sf->fn, strlen(sf->fn));
   1.256 				}else{
   1.257 					sf->dirreads++;
   1.258-					sendpkt("bus", SSH_FXP_READDIR, r->reqid, sf->hand, sf->handn);
   1.259+					sendpkt("bqs", SSH_FXP_READDIR, r, sf->hand, sf->handn);
   1.260 				}
   1.261 				wunlock(sf);
   1.262 			}else{
   1.263 				rlock(sf);
   1.264-				sendpkt("busvuu", SSH_FXP_READ, r->reqid, sf->hand, sf->handn,
   1.265+				sendpkt("bqsvuu", SSH_FXP_READ, r, sf->hand, sf->handn,
   1.266 					r->req->ifcall.offset, r->req->ifcall.count);
   1.267 				runlock(sf);
   1.268 			}
   1.269@@ -886,33 +896,33 @@ sendproc(void *)
   1.270 		case Twrite:
   1.271 			x = r->req->ifcall.count - r->req->ofcall.count;
   1.272 			if(x >= MAXWRITE) x = MAXWRITE;
   1.273+			r->req->ofcall.offset = x;
   1.274 			rlock(sf);
   1.275-			sendpkt("busvs", SSH_FXP_WRITE, r->reqid, sf->hand, sf->handn,
   1.276+			sendpkt("bqsvs", SSH_FXP_WRITE, r, sf->hand, sf->handn,
   1.277 				r->req->ifcall.offset + r->req->ofcall.count,
   1.278 				r->req->ifcall.data + r->req->ofcall.count,
   1.279 				x);
   1.280 			runlock(sf);
   1.281-			r->req->ofcall.offset = x;
   1.282 			break;
   1.283 		case Tstat:
   1.284 			rlock(sf);
   1.285 			r->req->d.name = finalelem(sf->fn);
   1.286 			r->req->d.qid = sf->qid;
   1.287 			if(sf->handn > 0 && (sf->qid.type & QTDIR) == 0)
   1.288-				sendpkt("bus", SSH_FXP_FSTAT, r->reqid, sf->hand, sf->handn);
   1.289+				sendpkt("bqs", SSH_FXP_FSTAT, r, sf->hand, sf->handn);
   1.290 			else
   1.291-				sendpkt("bus", SSH_FXP_STAT, r->reqid, sf->fn, strlen(sf->fn));
   1.292+				sendpkt("bqs", SSH_FXP_STAT, r, sf->fn, strlen(sf->fn));
   1.293 			runlock(sf);
   1.294 			break;
   1.295 		case Twstat:
   1.296-			if(r->req->aux == (void *) -1){
   1.297+			if(r->req->aux == (void*)-1){
   1.298 				rlock(sf);
   1.299 				s = parentdir(sf->fn);
   1.300 				t = pathcat(s, r->req->d.name);
   1.301+				r->req->aux = t;
   1.302+				sendpkt("bqss", SSH_FXP_RENAME, r, sf->fn, strlen(sf->fn), t, strlen(t));
   1.303+				runlock(sf);
   1.304 				free(s);
   1.305-				r->req->aux = t;
   1.306-				sendpkt("buss", SSH_FXP_RENAME, r->reqid, sf->fn, strlen(sf->fn), t, strlen(t));
   1.307-				runlock(sf);
   1.308 				break;
   1.309 			}
   1.310 			x = dir2attrib(&r->req->d, (uchar **) &s);
   1.311@@ -923,18 +933,18 @@ sendproc(void *)
   1.312 			}
   1.313 			rlock(sf);
   1.314 			if(sf->handn > 0)
   1.315-				sendpkt("bus[", SSH_FXP_FSETSTAT, r->reqid, sf->hand, sf->handn, s, x);
   1.316+				sendpkt("bqs[", SSH_FXP_FSETSTAT, r, sf->hand, sf->handn, s, x);
   1.317 			else
   1.318-				sendpkt("bus[", SSH_FXP_SETSTAT, r->reqid, sf->fn, strlen(sf->fn), s, x);
   1.319+				sendpkt("bqs[", SSH_FXP_SETSTAT, r, sf->fn, strlen(sf->fn), s, x);
   1.320 			runlock(sf);
   1.321 			free(s);
   1.322 			break;
   1.323 		case Tremove:
   1.324 			rlock(sf);
   1.325 			if((sf->qid.type & QTDIR) != 0)
   1.326-				sendpkt("bus", SSH_FXP_RMDIR, r->reqid, sf->fn, strlen(sf->fn));
   1.327+				sendpkt("bqs", SSH_FXP_RMDIR, r, sf->fn, strlen(sf->fn));
   1.328 			else
   1.329-				sendpkt("bus", SSH_FXP_REMOVE, r->reqid, sf->fn, strlen(sf->fn));
   1.330+				sendpkt("bqs", SSH_FXP_REMOVE, r, sf->fn, strlen(sf->fn));
   1.331 			runlock(sf);
   1.332 			break;
   1.333 		default:
   1.334@@ -983,7 +993,6 @@ recvproc(void *)
   1.335 		r = sreqrd[id];
   1.336 		if(r != nil){
   1.337 			sreqrd[id] = nil;
   1.338-			r->reqid = -1;
   1.339 			rwakeup(&sreqidrend);
   1.340 		}
   1.341 		qunlock(&sreqidlock);
   1.342@@ -992,13 +1001,14 @@ recvproc(void *)
   1.343 			continue;
   1.344 		}
   1.345 		if(r->closefid != nil){
   1.346+			putsfid(r->closefid);
   1.347 			putsreq(r);
   1.348 			continue;
   1.349 		}
   1.350 		if(r->req == nil)
   1.351 			sysfatal("recvproc: r->req == nil");
   1.352 
   1.353-		sf = r->req->fid != nil ? r->req->fid->aux : nil;
   1.354+		sf = r->req->fid->aux;
   1.355 		okresp = rxlen >= 9 && t == SSH_FXP_STATUS && GET4(rxpkt+5) == SSH_FX_OK;
   1.356 		switch(r->req->ifcall.type){
   1.357 		case Tattach:
   1.358@@ -1094,7 +1104,7 @@ recvproc(void *)
   1.359 				break;
   1.360 			}
   1.361 			if(r->req->aux == nil){
   1.362-				r->req->aux = (void *) -1;
   1.363+				r->req->aux = (void*)-1;
   1.364 				submitreq(r->req);
   1.365 			}else{
   1.366 				wlock(sf);
   1.367@@ -1199,7 +1209,6 @@ sshfsdestroyfid(Fid *f)
   1.368 		return;
   1.369 	if(sf->hand != nil){
   1.370 		sr = emalloc9p(sizeof(SReq));
   1.371-		sr->reqid = -1;
   1.372 		sr->closefid = sf;
   1.373 		submitsreq(sr);
   1.374 	}else