From: Peter Stephenson <p.w.stephenson@ntlworld.com>
To: zsh-workers@zsh.org
Subject: Re: Probabilistic crash on zsh 5.9 on x86_64
Date: Fri, 14 Apr 2023 09:29:18 +0100 (BST) [thread overview]
Message-ID: <300454748.2058336.1681460958361@mail.virginmedia.com> (raw)
In-Reply-To: <2002382304.4711947.1681404930419@mail.virginmedia.com>
> On 13/04/2023 17:55 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> > On 13/04/2023 17:40 Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
> > Even with this, shfunc is still freed, although shfunc->funcdef is not freed.
> > But doshfunc() continues to use shfunc, at lines 5854, 5954, 5957, etc.
> > If it doesn't crash, it means the freed memory is not yet used by others.
> > But I think this is unsafe, and valgrind gives lots of warnings.
>
> OK, so the starttrapscope() just above that point is pulling the rug out
> from under the function's feet. We've looked up the TRAPEXIT function and
> now that function's just been undefined.
>
> So some better save / restore for the shell function is probably needed.
I can't think of any major surgery I'd like to do for this special case
--- TRAPEXIT being from removed from within an execution of TRAPEXIT so
that the trap doesn't go off inside the function being executed.
Instead, here's a simple local fix-up which seems to keep valgrind quiet.
It's not the most elegant thing I can imagine.
diff --git a/Src/exec.c b/Src/exec.c
index 3b3d1235e..274800b10 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5779,12 +5779,25 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
char *name = shfunc->node.nam;
int flags = shfunc->node.flags;
char *fname = dupstring(name);
- Eprog prog;
+ Eprog prog, marked_prog;
static int oflags;
static int funcdepth;
Heap funcheap;
queue_signals(); /* Lots of memory and global state changes coming */
+ /*
+ * In case this is a special function such as a trap, mark it
+ * is in use right now, so it doesn't get freed early. The
+ * worst that can happen is this hangs around in memory a little
+ * longer than strictly needed.
+ *
+ * Classic example of this happening is running TRAPEXIT directly.
+ *
+ * Because the shell function's contents may change, we'll ensure
+ * we use a consistent structure for use / free.
+ */
+ marked_prog = shfunc->funcdef;
+ useeprog(marked_prog);
NEWHEAPS(funcheap) {
/*
@@ -5818,6 +5831,22 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
memcpy(funcsave->pipestats, pipestats, bytes);
}
+ if (!strcmp(fname, "TRAPEXIT")) {
+ /*
+ * If we are executing TRAPEXIT directly, starttrapscope()
+ * will pull the rug out from under us to ensure the
+ * exit trap isn't run inside the function. We just need
+ * the information locally here, so copy it on the heap.
+ *
+ * The funcdef is separately handled by reference counting.
+ */
+ Shfunc shcopy = (Shfunc)zhalloc(sizeof(struct shfunc));
+ memcpy(shcopy, shfunc, sizeof(struct shfunc));
+ shcopy->node.nam = dupstring(shfunc->node.nam);
+ shfunc = shcopy;
+ name = shfunc->node.nam;
+ }
+
starttrapscope();
startpatternscope();
@@ -5942,6 +5971,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
funcsave->fstack.filename = getshfuncfile(shfunc);
prog = shfunc->funcdef;
+ DPUTS1(!prog->nref, "function definition %s has zero reference count",
+ (fname && *fname) ? fname : "<anon>");
if (prog->flags & EF_RUN) {
Shfunc shf;
@@ -6046,6 +6077,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
}
} OLDHEAPS;
+ freeeprog(marked_prog);
unqueue_signals();
/*
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index e0b6afb5f..de57765a0 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -1083,6 +1083,17 @@ F:Must be tested with a top-level script rather than source or function
>trap1
# As of 5.7.1-test-2, the output was "out1 fn1 trap1 fn2" (on separate lines).
+ TRAPEXIT() { echo This is TRAPEXIT; }
+ TRAPEXIT
+ TRAPEXIT
+ TRAPEXIT
+0:No memory problems with explicit call to TRAPEXIT.
+>This is TRAPEXIT
+>This is TRAPEXIT
+>This is TRAPEXIT
+>This is TRAPEXIT
+# Three explicit calls, one implicit call at function exit.
+
%clean
rm -f TRAPEXIT
next prev parent reply other threads:[~2023-04-14 8:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-08 17:28 zsh bug report throwaway email thing
2023-04-08 21:36 ` Mikael Magnusson
2023-04-11 16:14 ` Jun. T
2023-04-11 16:29 ` Peter Stephenson
2023-04-13 10:47 ` Peter Stephenson
2023-04-13 11:12 ` Peter Stephenson
2023-04-13 13:02 ` Jun. T
2023-04-13 13:19 ` Peter Stephenson
2023-04-13 14:03 ` Peter Stephenson
2023-04-13 14:13 ` Peter Stephenson
2023-04-13 16:40 ` Jun. T
2023-04-13 16:55 ` Peter Stephenson
2023-04-14 8:29 ` Peter Stephenson [this message]
2023-04-14 12:21 ` Mikael Magnusson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=300454748.2058336.1681460958361@mail.virginmedia.com \
--to=p.w.stephenson@ntlworld.com \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).