From owner-freebsd-bugs@FreeBSD.ORG Mon Aug 10 13:50:01 2009 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1B88F1065670 for ; Mon, 10 Aug 2009 13:50:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id EBE458FC43 for ; Mon, 10 Aug 2009 13:50:00 +0000 (UTC) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id n7ADo01N059753 for ; Mon, 10 Aug 2009 13:50:00 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id n7ADo0Eb059752; Mon, 10 Aug 2009 13:50:00 GMT (envelope-from gnats) Resent-Date: Mon, 10 Aug 2009 13:50:00 GMT Resent-Message-Id: <200908101350.n7ADo0Eb059752@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Jilles Tjoelker Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 30FFA106566B for ; Mon, 10 Aug 2009 13:44:46 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107]) by mx1.freebsd.org (Postfix) with ESMTP id B9D3E8FC4C for ; Mon, 10 Aug 2009 13:44:45 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id C1729375BD7 for ; Mon, 10 Aug 2009 15:44:44 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id AF845228CC; Mon, 10 Aug 2009 15:44:44 +0200 (CEST) Message-Id: <20090810134444.AF845228CC@snail.stack.nl> Date: Mon, 10 Aug 2009 15:44:44 +0200 (CEST) From: Jilles Tjoelker To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Subject: bin/137640: [PATCH] sh(1) crash when redefining current function X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Jilles Tjoelker List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Aug 2009 13:50:01 -0000 >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@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 #include +#include /* * 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: