bin/137640: [PATCH] sh(1) crash when redefining current function

[Jilles Tjoelker]

>Number:         137640
>Category:       bin
>Synopsis:       [PATCH] sh(1) crash when redefining current function
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Mon Aug 10 13:50:00 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator:     Jilles Tjoelker
>Release:        FreeBSD 7.2-STABLE i386
>Organization:
None
>Environment:
System: FreeBSD snail.stack.nl 7.2-STABLE FreeBSD 7.2-STABLE #15: Thu Jun 11 11:26:27 CEST 2009 dean at meestal.stack.nl:/sabretooth.mnt/sources/7.x/i386/obj/sabretooth.mnt/sources/7.x/src/sys/STACK-SMP i386

also tested on 8.x
>Description:
sh(1) frees the memory of a function immediately when it is redefined or
unset, even if the function is currently being executed.
A use-after-free error.
>How-To-Repeat:
env MALLOC_OPTIONS=J sh -c 'g() { g() { :; }; :; }; g'
expected result: no output, return code 0
actual result: segmentation fault

Note that MALLOC_OPTIONS=J is default on -CURRENT.
>Fix:
Maintain a reference count on function definitions so they are kept until they
are no longer needed.

Idea is from dash.

--- func-redef-fix.patch begins here ---
Fix crash when undefining or redefining a currently executing function.
Memory may leak if multiple SIGINTs arrive in quick succession in
interactive mode, this will be fixed later by changing SIGINT handling.

diff --git a/eval.c b/eval.c
--- a/eval.c
+++ b/eval.c
@@ -791,6 +791,7 @@ evalcommand(union node *cmd, int flags, 
 		INTOFF;
 		savelocalvars = localvars;
 		localvars = NULL;
+		cmdentry.u.func->refcount++;
 		INTON;
 		savehandler = handler;
 		if (setjmp(jmploc.loc)) {
@@ -800,6 +801,7 @@ evalcommand(union node *cmd, int flags, 
 				freeparam(&shellparam);
 				shellparam = saveparam;
 			}
+			unreffunc(cmdentry.u.func);
 			poplocalvars();
 			localvars = savelocalvars;
 			handler = savehandler;
@@ -811,11 +813,12 @@ evalcommand(union node *cmd, int flags, 
 		funcnest++;
 		exitstatus = oexitstatus;
 		if (flags & EV_TESTED)
-			evaltree(cmdentry.u.func, EV_TESTED);
+			evaltree(&cmdentry.u.func->n, EV_TESTED);
 		else
-			evaltree(cmdentry.u.func, 0);
+			evaltree(&cmdentry.u.func->n, 0);
 		funcnest--;
 		INTOFF;
+		unreffunc(cmdentry.u.func);
 		poplocalvars();
 		localvars = savelocalvars;
 		freeparam(&shellparam);
diff --git a/exec.c b/exec.c
--- a/exec.c
+++ b/exec.c
@@ -286,7 +286,7 @@ printentry(struct tblentry *cmdp, int ve
 		out1fmt("function %s", cmdp->cmdname);
 		if (verbose) {
 			INTOFF;
-			name = commandtext(cmdp->param.func);
+			name = commandtext(&cmdp->param.func->n);
 			out1c(' ');
 			out1str(name);
 			ckfree(name);
@@ -583,7 +583,7 @@ deletefuncs(void)
 		while ((cmdp = *pp) != NULL) {
 			if (cmdp->cmdtype == CMDFUNCTION) {
 				*pp = cmdp->next;
-				freefunc(cmdp->param.func);
+				unreffunc(cmdp->param.func);
 				ckfree(cmdp);
 			} else {
 				pp = &cmdp->next;
@@ -670,7 +670,7 @@ addcmdentry(char *name, struct cmdentry 
 	INTOFF;
 	cmdp = cmdlookup(name, 1);
 	if (cmdp->cmdtype == CMDFUNCTION) {
-		freefunc(cmdp->param.func);
+		unreffunc(cmdp->param.func);
 	}
 	cmdp->cmdtype = entry->cmdtype;
 	cmdp->param = entry->u;
@@ -705,7 +705,7 @@ unsetfunc(char *name)
 	struct tblentry *cmdp;
 
 	if ((cmdp = cmdlookup(name, 0)) != NULL && cmdp->cmdtype == CMDFUNCTION) {
-		freefunc(cmdp->param.func);
+		unreffunc(cmdp->param.func);
 		delete_cmd_entry();
 		return (0);
 	}
diff --git a/exec.h b/exec.h
--- a/exec.h
+++ b/exec.h
@@ -50,7 +50,7 @@ struct cmdentry {
 	int cmdtype;
 	union param {
 		int index;
-		union node *func;
+		struct funcdef *func;
 	} u;
 	int special;
 };
diff --git a/mknodes.c b/mknodes.c
--- a/mknodes.c
+++ b/mknodes.c
@@ -248,8 +248,12 @@ output(char *file)
 	fputs("\tstruct nodelist *next;\n", hfile);
 	fputs("\tunion node *n;\n", hfile);
 	fputs("};\n\n\n", hfile);
-	fputs("union node *copyfunc(union node *);\n", hfile);
-	fputs("void freefunc(union node *);\n", hfile);
+	fputs("struct funcdef {\n", hfile);
+	fputs("\tunsigned int refcount;\n", hfile);
+	fputs("\tunion node n;\n", hfile);
+	fputs("};\n\n\n", hfile);
+	fputs("struct funcdef *copyfunc(union node *);\n", hfile);
+	fputs("void unreffunc(struct funcdef *);\n", hfile);
 
 	fputs(writer, cfile);
 	while (fgets(line, sizeof line, patfile) != NULL) {
diff --git a/nodes.c.pat b/nodes.c.pat
--- a/nodes.c.pat
+++ b/nodes.c.pat
@@ -35,6 +35,7 @@
 
 #include <sys/param.h>
 #include <stdlib.h>
+#include <stddef.h>
 /*
  * Routine for dealing with parsed shell commands.
  */
@@ -65,17 +66,22 @@ STATIC char *nodesavestr(char *);
  * Make a copy of a parse tree.
  */
 
-union node *
+struct funcdef *
 copyfunc(union node *n)
 {
+	struct funcdef *fn;
+
 	if (n == NULL)
 		return NULL;
-	funcblocksize = 0;
+	funcblocksize = offsetof(struct funcdef, n);
 	funcstringsize = 0;
 	calcsize(n);
-	funcblock = ckmalloc(funcblocksize + funcstringsize);
+	fn = ckmalloc(funcblocksize + funcstringsize);
+	fn->refcount = 1;
+	funcblock = (char *)fn + offsetof(struct funcdef, n);
 	funcstring = (char *)funcblock + funcblocksize;
-	return copynode(n);
+	copynode(n);
+	return fn;
 }
 
 
@@ -146,12 +152,17 @@ nodesavestr(char *s)
 
 
 /*
- * Free a parse tree.
+ * Decrement the reference count of a function definition, freeing it
+ * if it falls to 0.
  */
 
 void
-freefunc(union node *n)
+unreffunc(struct funcdef *fn)
 {
-	if (n)
-		ckfree(n);
+	if (fn) {
+		fn->refcount--;
+		if (fn->refcount > 0)
+			return;
+		ckfree(fn);
+	}
 }
--- func-redef-fix.patch ends here ---


>Release-Note:
>Audit-Trail:
>Unformatted: