shithub: riscv

Info  •  Files  •  Log  •  Branches

rio: make it harder to deadlock from kbdtap

It was quite easy for a single threaded
tap program to cause a deadlock. The
following describes the scenario:

prog: read a key from /dev/kbdtap
prog: processing key
rio: read a key from the 'upstream' /dev/kbd
rio: submit key to tap thread
prog: writes translated key back to /dev/kbdtap

Because the tap thread is blocked on waiting for
the program to read the new character, the program's
write never completes and we hit deadlock.

This was "conviently" avoided in testing with ktrans
because the reader and writer are in two different
processes. So in this case ktrans could make forward
progress.

To solve this we first turn the tap channels to have
a small buffer, this grases the wheel and allows the
above case to become tolerant for some amount of
new upstream characters.

Second we move the tap processing thread in to its
own process. This isolates this processing from the
rest of the main rio thread. This ensures that a misbehaving
kbdtap user will never lock you out of being able to 'Delete' it
and restore normal functionality.

--- a/sys/src/cmd/rio/rio.c

+++ b/sys/src/cmd/rio/rio.c

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

 int		threadrforkflag = 0;	/* should be RFENVG but that hides rio from plumber */

 void	mousethread(void*);

-void	keyboardthread(void*);

+void	keyboardtap(void*);

 void	winclosethread(void*);

 void	initcmd(void*);

 Channel* initkbd(void);

@@ -196,8 +196,9 @@

 	kbdchan = initkbd();

 	if(kbdchan == nil)

 		error("can't find keyboard");

-	totap = chancreate(sizeof(char*), 0);

-	fromtap = chancreate(sizeof(char*), 0);

+	totap = chancreate(sizeof(char*), 32);

+	fromtap = chancreate(sizeof(char*), 32);

+	proccreate(keyboardtap, nil, STACK);

 	wscreen = allocscreen(screen, background, 0);

 	if(wscreen == nil)

@@ -206,7 +207,6 @@

 	flushimage(display, 1);

 	timerinit();

-	threadcreate(keyboardthread, nil, STACK);

 	threadcreate(mousethread, nil, STACK);

 	threadcreate(winclosethread, nil, STACK);

 	filsys = filsysinit(xfidinit());

@@ -337,7 +337,7 @@

 }

 void

-keyboardthread(void*)

+keyboardtap(void*)

 {

 	char *s;

 	Channel *out;

@@ -345,7 +345,7 @@

 	enum { Kdev, Ktap, NALT};

 	enum { Mnorm, Mtap };

-	threadsetname("keyboardthread");	

+	threadsetname("keyboardtap");	

 	static Alt alts[NALT+1];

 	alts[Kdev].c = kbdchan;

--- a/sys/src/cmd/rio/xfid.c

+++ b/sys/src/cmd/rio/xfid.c

@@ -575,8 +575,14 @@

 			return;

 		}

 		e = x->data + cnt;

-		for(p = x->data; p < e; p += strlen(p)+1)

+		for(p = x->data; p < e; p += strlen(p)+1){

+			if(*p == '\0'){

+				fc.count = p - x->data;

+				filsysrespond(x->fs, x, &fc, "null message type");

+				return;

+			}

 			chanprint(fromtap, "%s", p);

+		}

 		break;

 	default:

@@ -704,6 +710,7 @@

 		alts[Aflush].v = nil;

 		alts[Aflush].op = CHANRCV;

 		alts[Aend].op = CHANEND;

+

 		switch(alt(alts)){

 		case Adata:

 			break;

@@ -715,6 +722,7 @@

 			return;

 		}

 		fc.data = t;

+		/* kbdproc ensures we're only dealing with one message */

 		fc.count = strlen(t)+1;

 		filsysrespond(x->fs, x, &fc, nil);

 		free(t);