Re: [9front] fix race condition on writes to #v/vgactl

[qwx@sciops.net]

On Wed Aug 17 14:17:35 +0200 2022, ori@eigenstate.org wrote:
> Quoth qwx@sciops.net:
> >  	case CMdrawinit:
> > -		if(scr->gscreen == nil)
> > +		canqlock(&drawlock);
> > +		if(scr->gscreen == nil){
> > +			qunlock(&drawlock);
> >  			error(Enoscreen);
> > +		}
> 
> this canqlock without an if looks supsicious to me;
> if someone else holds the lock, the canqlock fails,
> but that means we unlock *someone elses* lock.

You're absolutely right.  Here's another attempt; the code is a mess,
but I'm not up for refactoring it all right now.

What do you think?

Thanks,
qwx


diff d86a7ed412555192e2000a9a34b3372f380ec3d0 uncommitted
--- a//sys/src/9/pc/devvga.c
+++ b//sys/src/9/pc/devvga.c
@@ -280,8 +280,10 @@
 			error("bad channel");
 		if(chantodepth(chan) != z)
 			error("depth, channel do not match");
+		qlock(&drawlock);
 		deletescreenimage();
 		setscreensize(scr, r.max.x, r.max.y, z, chan, scr->tilt);
+		qunlock(&drawlock);
 		return;
 
 	case CMactualsize:
@@ -296,6 +298,7 @@
 			error(Ebadarg);
 		if(r.max.x > scr->width || r.max.y > scr->height)
 			error("physical screen bigger than virtual");
+		qlock(&drawlock);
 		deletescreenimage();
 		setactualsize(scr, r);
 		goto Resized;
@@ -324,6 +327,7 @@
 		}
 		if(scr->gscreen == nil)
 			return;
+		qlock(&drawlock);
 		r = actualscreensize(scr);
 		chan = scr->gscreen->chan;
 		z = scr->gscreen->depth;
@@ -331,9 +335,11 @@
 		setscreensize(scr, scr->width, scr->height, z, chan, tilt);
 		setactualsize(scr, r);
 		/* no break */
-	case CMdrawinit:
-		if(scr->gscreen == nil)
+	Init:
+		if(scr->gscreen == nil){
+			qunlock(&drawlock);
 			error(Enoscreen);
+		}
 		if(scr->dev && scr->dev->drawinit)
 			scr->dev->drawinit(scr);
 		hwblank = scr->blank != nil;
@@ -341,7 +347,12 @@
 	Resized:
 		vgascreenwin(scr);
 		resetscreenimage();
+		qunlock(&drawlock);
 		return;
+
+	case CMdrawinit:
+		qlock(&drawlock);
+		goto Init;
 
 	case CMlinear:
 		if(cb->nf!=2 && cb->nf!=3)
--- a//sys/src/9/pc/screen.c
+++ b//sys/src/9/pc/screen.c
@@ -64,17 +64,11 @@
 void
 setactualsize(VGAscr *scr, Rectangle r)
 {
-	qlock(&drawlock);
-
 	r.min = ZP;
 	r.max = tiltsize(scr->tilt, r.max);
-	if(rectclip(&r, scr->gscreen->r) == 0){
-		qunlock(&drawlock);
+	if(rectclip(&r, scr->gscreen->r) == 0)
 		return;
-	}
 	scr->gscreen->clipr = r;
-
-	qunlock(&drawlock);
 }
 
 static char*
@@ -136,11 +130,8 @@
 {
 	char *err;
 
-	qlock(&drawlock);
-	if(waserror()){
-		qunlock(&drawlock);
+	if(waserror())
 		nexterror();
-	}
 
 	if(memimageinit() < 0)
 		error("memimageinit failed");
@@ -169,7 +160,6 @@
 		cursoron();
 	}
 
-	qunlock(&drawlock);
 	poperror();
 }
 
--- a//sys/src/9/pc/vga.c
+++ b//sys/src/9/pc/vga.c
@@ -183,8 +183,6 @@
 	Point p;
 	int h;
 
-	qlock(&drawlock);
-	
 	h = scr->memdefont->height;
 	r = scr->gscreen->r;
 
@@ -215,11 +213,7 @@
 	curpos = window.min;
 
 	flushmemscreen(r);
-
-	qunlock(&drawlock);
-
 	vgascreenputs(kmesg.buf, kmesg.n);
-
 	screenputs = vgascreenputs;
 }
 
--- a//sys/src/9/port/devdraw.c
+++ b//sys/src/9/port/devdraw.c
@@ -949,7 +949,6 @@
 void
 deletescreenimage(void)
 {
-	dlock();
 	if(screenimage){
 		/* will be freed via screendimage; disable */
 		screenimage->clipr = ZR;
@@ -959,15 +958,12 @@
 		drawfreedimage(screendimage);
 		screendimage = nil;
 	}
-	dunlock();
 }
 
 void
 resetscreenimage(void)
 {
-	dlock();
 	initscreenimage();
-	dunlock();
 }
 
 static Chan*
--- a//sys/src/9/zynq/screen.c
+++ b//sys/src/9/zynq/screen.c
@@ -299,8 +299,8 @@
 		if(chantodepth(chan) != z)
 			error("depth, channel do not match");
 
-		deletescreenimage();
 		eqlock(&drawlock);
+		deletescreenimage();
 		if(memimageinit() < 0){
 			qunlock(&drawlock);
 			error("memimageinit failed");
@@ -310,14 +310,18 @@
 			gscreen = nil;
 		}
 		gscreen = allocmemimage(Rect(0,0,x,y), chan);
-		qunlock(&drawlock);
-		/* wet floor */
-
-	case CMinit:
-		if(gscreen == nil)
+	Init:
+		if(gscreen == nil){
+			qunlock(&drawlock);
 			error("no framebuffer");
+		}
 		resetscreenimage();
+		qunlock(&drawlock);
 		break;
+
+	case CMinit:
+		qlock(&drawlock);
+		goto Init;
 	}
 	free(cb);
 	poperror();