shithub: riscv

Info  •  Files  •  Log  •  Branches

kernel: attachimage / exec error handling

attachimage()'s approach to handling newseg() error is flawed:

a) the the image is on the hash table, but ref is still 0, and
there is no segment/pages attached to it so nobody is going to
reclaim / putimage() it -> leak

b) calling pexit() would deadlock us because exec has acquired
up->seglock when calling attachimage(), so this would just deadlock.

the fix does the following:

attachimage() will putimage() and nexterror() if newseg() fails
instead of pexit(). this is less surprising.

exec now keeps the condition variable commit which is set once
we are commited / reached the point of no return and check this
variable in the highest waserror() handler and pexit() us there.

this way we have released up all the locks and pexit() will
cleanup.

note: this bug shouldnt us hit in with the current newseg()
implementation as it uses smalloc() which would wait to
satisfy the allocation instead of erroring.

--- a/sys/src/9/port/segment.c

+++ b/sys/src/9/port/segment.c

@@ -285,14 +285,14 @@

 	unlock(&imagealloc);

 	if(i->s == 0) {

-		/* Disaster after commit in exec */

+		i->ref++;

 		if(waserror()) {

 			unlock(i);

-			pexit(Enovmem, 1);

+			putimage(i);

+			nexterror();

 		}

 		i->s = newseg(type, base, len);

 		i->s->image = i;

-		i->ref++;

 		poperror();

 	}

 	else

--- a/sys/src/9/port/sysproc.c

+++ b/sys/src/9/port/sysproc.c

@@ -228,7 +228,7 @@

 	char *a, *charp, *args, *file, *file0;

 	char *progarg[sizeof(Exec)/2+1], *elem, progelem[64];

 	ulong ssize, spage, nargs, nbytes, n, bssend;

-	int indir;

+	int indir, commit;

 	Exec exec;

 	char line[sizeof(Exec)];

 	Fgrp *f;

@@ -236,6 +236,7 @@

 	ulong magic, text, entry, data, bss;

 	Tos *tos;

+	commit = 0;

 	indir = 0;

 	elem = nil;

 	validaddr(arg[0], 1, 0);

@@ -243,6 +244,9 @@

 	if(waserror()){

 		free(file0);

 		free(elem);

+		/* Disaster after commit */

+		if(commit)

+			pexit(up->errstr, 1);

 		nexterror();

 	}

 	file = file0;

@@ -407,6 +411,9 @@

 	}

 	up->nargs = n;

+	commit = 1;

+	USED(commit);

+

 	/*

 	 * Committed.

 	 * Free old memory.

@@ -466,7 +473,6 @@

 	up->seg[SSEG] = s;

 	qunlock(&up->seglock);

 	poperror();	/* seglock */

-	poperror();	/* elem */

 	/*

 	 *  '/' processes are higher priority (hack to make /ip more responsive).

@@ -474,8 +480,9 @@

 	if(devtab[tc->type]->dc == L'/')

 		up->basepri = PriRoot;

 	up->priority = up->basepri;

-	poperror();

 	cclose(tc);

+	poperror();	/* tc */

+	poperror();	/* elem */

 	qlock(&up->debug);

 	up->nnote = 0;