From jilles@stack.nl Mon Aug 10 13:44:46 2009 Return-Path: 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 Reply-To: Jilles Tjoelker To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: [PATCH] sh(1) crash when redefining current function X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 137640 >Category: bin >Synopsis: [PATCH] sh(1) crash when redefining current function >Confidential: no >Severity: serious >Priority: medium >Responsible: jilles >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Mon Aug 10 13:50:00 UTC 2009 >Closed-Date: Tue Dec 08 23:49:22 UTC 2009 >Last-Modified: Tue Dec 08 23:49:22 UTC 2009 >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: From: Jilles Tjoelker To: bug-followup@FreeBSD.org, jilles@stack.nl Cc: Subject: Re: bin/137640: [PATCH] sh(1) crash when redefining current function Date: Fri, 14 Aug 2009 16:17:24 +0200 --azLHFNyN32YCQGCU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline That patch does not work, try this. --azLHFNyN32YCQGCU Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="func-redef-fix.patch" Fix crash when undefining or redefining a currently executing function. Memory may leak if multiple SIGINTs arrive 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; + reffunc(cmdentry.u.func); 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 @@ -46,11 +46,12 @@ enum { TYPECMD_TYPE /* type */ }; +union node; 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,13 @@ 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 reffunc(struct funcdef *);\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); - funcstring = (char *)funcblock + funcblocksize; - return copynode(n); + fn = ckmalloc(funcblocksize + funcstringsize); + fn->refcount = 1; + funcblock = (char *)fn + offsetof(struct funcdef, n); + funcstring = (char *)fn + funcblocksize; + copynode(n); + return fn; } @@ -144,14 +150,25 @@ nodesavestr(char *s) } +void +reffunc(struct funcdef *fn) +{ + fn->refcount++; +} + /* - * 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); + } } --azLHFNyN32YCQGCU-- From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/137640: commit references a PR Date: Sun, 23 Aug 2009 21:10:09 +0000 (UTC) Author: jilles Date: Sun Aug 23 21:09:46 2009 New Revision: 196483 URL: http://svn.freebsd.org/changeset/base/196483 Log: sh: Fix crash when undefining or redefining a currently executing function. Add a reference count to function definitions. Memory may leak if multiple SIGINTs arrive in interactive mode, this will be fixed later by changing SIGINT handling. PR: bin/137640 Added: head/tools/regression/bin/sh/execution/func1.0 (contents, props changed) Modified: head/bin/sh/eval.c head/bin/sh/exec.c head/bin/sh/exec.h head/bin/sh/mknodes.c head/bin/sh/nodes.c.pat Modified: head/bin/sh/eval.c ============================================================================== --- head/bin/sh/eval.c Sun Aug 23 21:00:21 2009 (r196482) +++ head/bin/sh/eval.c Sun Aug 23 21:09:46 2009 (r196483) @@ -785,6 +785,7 @@ evalcommand(union node *cmd, int flags, INTOFF; savelocalvars = localvars; localvars = NULL; + reffunc(cmdentry.u.func); INTON; savehandler = handler; if (setjmp(jmploc.loc)) { @@ -794,6 +795,7 @@ evalcommand(union node *cmd, int flags, freeparam(&shellparam); shellparam = saveparam; } + unreffunc(cmdentry.u.func); poplocalvars(); localvars = savelocalvars; handler = savehandler; @@ -805,11 +807,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); Modified: head/bin/sh/exec.c ============================================================================== --- head/bin/sh/exec.c Sun Aug 23 21:00:21 2009 (r196482) +++ head/bin/sh/exec.c Sun Aug 23 21:09:46 2009 (r196483) @@ -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); } Modified: head/bin/sh/exec.h ============================================================================== --- head/bin/sh/exec.h Sun Aug 23 21:00:21 2009 (r196482) +++ head/bin/sh/exec.h Sun Aug 23 21:09:46 2009 (r196483) @@ -46,11 +46,12 @@ enum { TYPECMD_TYPE /* type */ }; +union node; struct cmdentry { int cmdtype; union param { int index; - union node *func; + struct funcdef *func; } u; int special; }; Modified: head/bin/sh/mknodes.c ============================================================================== --- head/bin/sh/mknodes.c Sun Aug 23 21:00:21 2009 (r196482) +++ head/bin/sh/mknodes.c Sun Aug 23 21:09:46 2009 (r196483) @@ -248,8 +248,13 @@ 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 reffunc(struct funcdef *);\n", hfile); + fputs("void unreffunc(struct funcdef *);\n", hfile); fputs(writer, cfile); while (fgets(line, sizeof line, patfile) != NULL) { Modified: head/bin/sh/nodes.c.pat ============================================================================== --- head/bin/sh/nodes.c.pat Sun Aug 23 21:00:21 2009 (r196482) +++ head/bin/sh/nodes.c.pat Sun Aug 23 21:09:46 2009 (r196483) @@ -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); - funcstring = (char *)funcblock + funcblocksize; - return copynode(n); + fn = ckmalloc(funcblocksize + funcstringsize); + fn->refcount = 1; + funcblock = (char *)fn + offsetof(struct funcdef, n); + funcstring = (char *)fn + funcblocksize; + copynode(n); + return fn; } @@ -144,14 +150,25 @@ nodesavestr(char *s) } +void +reffunc(struct funcdef *fn) +{ + fn->refcount++; +} + /* - * 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); + } } Added: head/tools/regression/bin/sh/execution/func1.0 ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/tools/regression/bin/sh/execution/func1.0 Sun Aug 23 21:09:46 2009 (r196483) @@ -0,0 +1,4 @@ +# $FreeBSD$ + +MALLOC_OPTIONS=J sh -c 'g() { g() { :; }; :; }; g' && +MALLOC_OPTIONS=J sh -c 'g() { unset -f g; :; }; g' _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State-Changed-From-To: open->patched State-Changed-By: jilles State-Changed-When: Sun Sep 20 21:15:46 UTC 2009 State-Changed-Why: Working fine in -CURRENT (with r196634). Responsible-Changed-From-To: freebsd-bugs->jilles Responsible-Changed-By: jilles Responsible-Changed-When: Sun Sep 20 21:15:46 UTC 2009 Responsible-Changed-Why: Take. http://www.freebsd.org/cgi/query-pr.cgi?pr=137640 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/137640: commit references a PR Date: Sun, 11 Oct 2009 16:35:22 +0000 (UTC) Author: jilles Date: Sun Oct 11 16:35:12 2009 New Revision: 197959 URL: http://svn.freebsd.org/changeset/base/197959 Log: MFC r196483,r196634: sh: Fix crash when undefining or redefining a currently executing function Add a reference count to function definitions. Memory may leak if a SIGINT arrives in interactive mode at exactly the wrong time, this will be fixed later by changing SIGINT handling. PR: bin/137640 Approved by: re (kib) Added: stable/8/tools/regression/bin/sh/execution/func1.0 - copied unchanged from r196483, head/tools/regression/bin/sh/execution/func1.0 stable/8/tools/regression/bin/sh/execution/func2.0 - copied unchanged from r196634, head/tools/regression/bin/sh/execution/func2.0 Modified: stable/8/bin/sh/ (props changed) stable/8/bin/sh/eval.c stable/8/bin/sh/exec.c stable/8/bin/sh/exec.h stable/8/bin/sh/mknodes.c stable/8/bin/sh/nodes.c.pat stable/8/tools/regression/bin/sh/ (props changed) Modified: stable/8/bin/sh/eval.c ============================================================================== --- stable/8/bin/sh/eval.c Sun Oct 11 16:23:11 2009 (r197958) +++ stable/8/bin/sh/eval.c Sun Oct 11 16:35:12 2009 (r197959) @@ -785,6 +785,7 @@ evalcommand(union node *cmd, int flags, INTOFF; savelocalvars = localvars; localvars = NULL; + reffunc(cmdentry.u.func); INTON; savehandler = handler; if (setjmp(jmploc.loc)) { @@ -794,6 +795,7 @@ evalcommand(union node *cmd, int flags, freeparam(&shellparam); shellparam = saveparam; } + unreffunc(cmdentry.u.func); poplocalvars(); localvars = savelocalvars; handler = savehandler; @@ -805,11 +807,12 @@ evalcommand(union node *cmd, int flags, funcnest++; exitstatus = oexitstatus; if (flags & EV_TESTED) - evaltree(cmdentry.u.func, EV_TESTED); + evaltree(getfuncnode(cmdentry.u.func), EV_TESTED); else - evaltree(cmdentry.u.func, 0); + evaltree(getfuncnode(cmdentry.u.func), 0); funcnest--; INTOFF; + unreffunc(cmdentry.u.func); poplocalvars(); localvars = savelocalvars; freeparam(&shellparam); Modified: stable/8/bin/sh/exec.c ============================================================================== --- stable/8/bin/sh/exec.c Sun Oct 11 16:23:11 2009 (r197958) +++ stable/8/bin/sh/exec.c Sun Oct 11 16:35:12 2009 (r197959) @@ -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(getfuncnode(cmdp->param.func)); 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); } Modified: stable/8/bin/sh/exec.h ============================================================================== --- stable/8/bin/sh/exec.h Sun Oct 11 16:23:11 2009 (r197958) +++ stable/8/bin/sh/exec.h Sun Oct 11 16:35:12 2009 (r197959) @@ -46,11 +46,12 @@ enum { TYPECMD_TYPE /* type */ }; +union node; struct cmdentry { int cmdtype; union param { int index; - union node *func; + struct funcdef *func; } u; int special; }; Modified: stable/8/bin/sh/mknodes.c ============================================================================== --- stable/8/bin/sh/mknodes.c Sun Oct 11 16:23:11 2009 (r197958) +++ stable/8/bin/sh/mknodes.c Sun Oct 11 16:35:12 2009 (r197959) @@ -248,8 +248,11 @@ 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("struct funcdef *copyfunc(union node *);\n", hfile); + fputs("union node *getfuncnode(struct funcdef *);\n", hfile); + fputs("void reffunc(struct funcdef *);\n", hfile); + fputs("void unreffunc(struct funcdef *);\n", hfile); fputs(writer, cfile); while (fgets(line, sizeof line, patfile) != NULL) { Modified: stable/8/bin/sh/nodes.c.pat ============================================================================== --- stable/8/bin/sh/nodes.c.pat Sun Oct 11 16:23:11 2009 (r197958) +++ stable/8/bin/sh/nodes.c.pat Sun Oct 11 16:35:12 2009 (r197959) @@ -35,6 +35,7 @@ #include #include +#include /* * Routine for dealing with parsed shell commands. */ @@ -60,25 +61,40 @@ STATIC struct nodelist *copynodelist(str STATIC char *nodesavestr(char *); +struct funcdef { + unsigned int refcount; + union node n; +}; /* * 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); - funcstring = (char *)funcblock + funcblocksize; - return copynode(n); + fn = ckmalloc(funcblocksize + funcstringsize); + fn->refcount = 1; + funcblock = (char *)fn + offsetof(struct funcdef, n); + funcstring = (char *)fn + funcblocksize; + copynode(n); + return fn; } +union node * +getfuncnode(struct funcdef *fn) +{ + return fn == NULL ? NULL : &fn->n; +} + STATIC void calcsize(union node *n) @@ -144,14 +160,26 @@ nodesavestr(char *s) } +void +reffunc(struct funcdef *fn) +{ + if (fn) + fn->refcount++; +} + /* - * 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); + } } Copied: stable/8/tools/regression/bin/sh/execution/func1.0 (from r196483, head/tools/regression/bin/sh/execution/func1.0) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/8/tools/regression/bin/sh/execution/func1.0 Sun Oct 11 16:35:12 2009 (r197959, copy of r196483, head/tools/regression/bin/sh/execution/func1.0) @@ -0,0 +1,4 @@ +# $FreeBSD$ + +MALLOC_OPTIONS=J sh -c 'g() { g() { :; }; :; }; g' && +MALLOC_OPTIONS=J sh -c 'g() { unset -f g; :; }; g' Copied: stable/8/tools/regression/bin/sh/execution/func2.0 (from r196634, head/tools/regression/bin/sh/execution/func2.0) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/8/tools/regression/bin/sh/execution/func2.0 Sun Oct 11 16:35:12 2009 (r197959, copy of r196634, head/tools/regression/bin/sh/execution/func2.0) @@ -0,0 +1,11 @@ +# $FreeBSD$ + +f() { } +f +hash -v f >/dev/null +f() { { }; } +f +hash -v f >/dev/null +f() { { } } +f +hash -v f >/dev/null _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/137640: commit references a PR Date: Tue, 8 Dec 2009 22:37:26 +0000 (UTC) Author: jilles Date: Tue Dec 8 22:37:07 2009 New Revision: 200279 URL: http://svn.freebsd.org/changeset/base/200279 Log: MFC r196483,r196634: sh: Fix crash when undefining or redefining a currently executing function Add a reference count to function definitions. Memory may leak if a SIGINT arrives in interactive mode at exactly the wrong time. PR: bin/137640 Added: stable/7/tools/regression/bin/sh/execution/ stable/7/tools/regression/bin/sh/execution/func1.0 - copied unchanged from r196483, head/tools/regression/bin/sh/execution/func1.0 stable/7/tools/regression/bin/sh/execution/func2.0 - copied unchanged from r196634, head/tools/regression/bin/sh/execution/func2.0 Modified: stable/7/bin/sh/eval.c stable/7/bin/sh/exec.c stable/7/bin/sh/exec.h stable/7/bin/sh/mknodes.c stable/7/bin/sh/nodes.c.pat Directory Properties: stable/7/bin/sh/ (props changed) stable/7/tools/regression/bin/sh/ (props changed) Modified: stable/7/bin/sh/eval.c ============================================================================== --- stable/7/bin/sh/eval.c Tue Dec 8 22:35:39 2009 (r200278) +++ stable/7/bin/sh/eval.c Tue Dec 8 22:37:07 2009 (r200279) @@ -773,6 +773,7 @@ evalcommand(union node *cmd, int flags, INTOFF; savelocalvars = localvars; localvars = NULL; + reffunc(cmdentry.u.func); INTON; if (setjmp(jmploc.loc)) { if (exception == EXSHELLPROC) @@ -781,6 +782,7 @@ evalcommand(union node *cmd, int flags, freeparam(&shellparam); shellparam = saveparam; } + unreffunc(cmdentry.u.func); poplocalvars(); localvars = savelocalvars; handler = savehandler; @@ -792,11 +794,12 @@ evalcommand(union node *cmd, int flags, mklocal(sp->text); funcnest++; if (flags & EV_TESTED) - evaltree(cmdentry.u.func, EV_TESTED); + evaltree(getfuncnode(cmdentry.u.func), EV_TESTED); else - evaltree(cmdentry.u.func, 0); + evaltree(getfuncnode(cmdentry.u.func), 0); funcnest--; INTOFF; + unreffunc(cmdentry.u.func); poplocalvars(); localvars = savelocalvars; freeparam(&shellparam); Modified: stable/7/bin/sh/exec.c ============================================================================== --- stable/7/bin/sh/exec.c Tue Dec 8 22:35:39 2009 (r200278) +++ stable/7/bin/sh/exec.c Tue Dec 8 22:37:07 2009 (r200279) @@ -285,7 +285,7 @@ printentry(struct tblentry *cmdp, int ve out1fmt("function %s", cmdp->cmdname); if (verbose) { INTOFF; - name = commandtext(cmdp->param.func); + name = commandtext(getfuncnode(cmdp->param.func)); out1c(' '); out1str(name); ckfree(name); @@ -582,7 +582,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; @@ -669,7 +669,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; @@ -704,7 +704,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); } Modified: stable/7/bin/sh/exec.h ============================================================================== --- stable/7/bin/sh/exec.h Tue Dec 8 22:35:39 2009 (r200278) +++ stable/7/bin/sh/exec.h Tue Dec 8 22:37:07 2009 (r200279) @@ -46,11 +46,12 @@ enum { TYPECMD_TYPE /* type */ }; +union node; struct cmdentry { int cmdtype; union param { int index; - union node *func; + struct funcdef *func; } u; int special; }; Modified: stable/7/bin/sh/mknodes.c ============================================================================== --- stable/7/bin/sh/mknodes.c Tue Dec 8 22:35:39 2009 (r200278) +++ stable/7/bin/sh/mknodes.c Tue Dec 8 22:37:07 2009 (r200279) @@ -248,8 +248,11 @@ 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("struct funcdef *copyfunc(union node *);\n", hfile); + fputs("union node *getfuncnode(struct funcdef *);\n", hfile); + fputs("void reffunc(struct funcdef *);\n", hfile); + fputs("void unreffunc(struct funcdef *);\n", hfile); fputs(writer, cfile); while (fgets(line, sizeof line, patfile) != NULL) { Modified: stable/7/bin/sh/nodes.c.pat ============================================================================== --- stable/7/bin/sh/nodes.c.pat Tue Dec 8 22:35:39 2009 (r200278) +++ stable/7/bin/sh/nodes.c.pat Tue Dec 8 22:37:07 2009 (r200279) @@ -35,6 +35,7 @@ #include #include +#include /* * Routine for dealing with parsed shell commands. */ @@ -60,25 +61,40 @@ STATIC struct nodelist *copynodelist(str STATIC char *nodesavestr(char *); +struct funcdef { + unsigned int refcount; + union node n; +}; /* * 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); - funcstring = (char *)funcblock + funcblocksize; - return copynode(n); + fn = ckmalloc(funcblocksize + funcstringsize); + fn->refcount = 1; + funcblock = (char *)fn + offsetof(struct funcdef, n); + funcstring = (char *)fn + funcblocksize; + copynode(n); + return fn; } +union node * +getfuncnode(struct funcdef *fn) +{ + return fn == NULL ? NULL : &fn->n; +} + STATIC void calcsize(union node *n) @@ -144,14 +160,26 @@ nodesavestr(char *s) } +void +reffunc(struct funcdef *fn) +{ + if (fn) + fn->refcount++; +} + /* - * 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); + } } Copied: stable/7/tools/regression/bin/sh/execution/func1.0 (from r196483, head/tools/regression/bin/sh/execution/func1.0) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/7/tools/regression/bin/sh/execution/func1.0 Tue Dec 8 22:37:07 2009 (r200279, copy of r196483, head/tools/regression/bin/sh/execution/func1.0) @@ -0,0 +1,4 @@ +# $FreeBSD$ + +MALLOC_OPTIONS=J sh -c 'g() { g() { :; }; :; }; g' && +MALLOC_OPTIONS=J sh -c 'g() { unset -f g; :; }; g' Copied: stable/7/tools/regression/bin/sh/execution/func2.0 (from r196634, head/tools/regression/bin/sh/execution/func2.0) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/7/tools/regression/bin/sh/execution/func2.0 Tue Dec 8 22:37:07 2009 (r200279, copy of r196634, head/tools/regression/bin/sh/execution/func2.0) @@ -0,0 +1,11 @@ +# $FreeBSD$ + +f() { } +f +hash -v f >/dev/null +f() { { }; } +f +hash -v f >/dev/null +f() { { } } +f +hash -v f >/dev/null _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State-Changed-From-To: patched->closed State-Changed-By: jilles State-Changed-When: Tue Dec 8 23:49:21 UTC 2009 State-Changed-Why: Fixed. http://www.freebsd.org/cgi/query-pr.cgi?pr=137640 >Unformatted: