[PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[m0viefreak]

Using PAT_STATIC, any of the functions called in the hook that make
use of patterns, implicitly change the pattern we use in the
isearch loop. In the best case this results in pattern-not-found,
in the worst case we get a dump.

Use PAT_ZDUP instead and take care of freeing it again.

Minimal reproducing example:

% bindkey '^R' history-incremental-pattern-search-backward
% evil_hook() { a=(); : ${a[(r)foo*]}; };
% zle -N zle-isearch-update evil_hook
% : foo
% : bar
% : baz
%

type: <^R>b
% : baz
bck-i-search: b_

type: <^R>
% : foo
bck-i-search: b_

': foo' is found instead of ': bar' because evil_hook modified the
static pattern used in isearch.

Related: zsh-syntax-highlighting issue which found this bug:
https://github.com/zsh-users/zsh-syntax-highlighting/issues/407
---
 Src/Zle/zle_hist.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Src/Zle/zle_hist.c b/Src/Zle/zle_hist.c
index abd6e17..434735d 100644
--- a/Src/Zle/zle_hist.c
+++ b/Src/Zle/zle_hist.c
@@ -1220,13 +1220,12 @@ doisearch(char **args, int dir, int pattern)
 		char *patbuf = ztrdup(sbuf);
 		char *patstring;
 		/*
-		 * Use static pattern buffer since we don't need
-		 * to maintain it and won't call other pattern functions
-		 * meanwhile.
+		 * Do not use static pattern buffer (PAT_STATIC) since we call zle hooks,
+		 * which might call other pattern functions. Use PAT_ZDUP instead.
 		 * Use PAT_NOANCH because we don't need the match
 		 * anchored to the end, even if it is at the start.
 		 */
-		int patflags = PAT_STATIC|PAT_NOANCH;
+		int patflags = PAT_ZDUP|PAT_NOANCH;
 		if (sbuf[0] == '^') {
 		    /*
 		     * We'll handle the anchor later when
@@ -1521,6 +1520,7 @@ doisearch(char **args, int dir, int pattern)
 		    if (only_one || !top_spot || old_sbptr != sbptr)
 			break;
 		}
+		freepatprog(patprog);
 		patprog = NULL;
 		nosearch = 1;
 		skip_pos = 0;
@@ -1632,6 +1632,7 @@ doisearch(char **args, int dir, int pattern)
 	    }
 	    strcpy(sbuf + sbptr, paste);
 	    sbptr += pastelen;
+	    freepatprog(patprog);
 	    patprog = NULL;
 	    free(paste);
 	} else if (cmd == Th(z_acceptsearch)) {
@@ -1682,6 +1683,7 @@ doisearch(char **args, int dir, int pattern)
 	     * always valid at this point.
 	     */
 	    sbptr += zlecharasstring(LASTFULLCHAR, sbuf + sbptr);
+	    freepatprog(patprog);
 	    patprog = NULL;
 	}
 	if (feep)
@@ -1702,6 +1704,7 @@ doisearch(char **args, int dir, int pattern)
     zsfree(okeymap);
     if (matchlist)
 	freematchlist(matchlist);
+    freepatprog(patprog);
     isearch_active = 0;
     /*
      * Don't allow unused characters provided as a string to the
-- 
2.8.3

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Jan 6,  5:25pm, m0viefreak wrote:
}
} Using PAT_STATIC, any of the functions called in the hook that make
} use of patterns, implicitly change the pattern we use in the
} isearch loop. In the best case this results in pattern-not-found,
} in the worst case we get a dump.

Thanks for catching this.  I expect we're going to be findind this
kind of thing elsewhere, now that we have so many hook points -- lots
of code that was written expecting it would never be re-entered is
potentially going to be invoked from hooks, possibly even from hooks
within hooks.

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Jan 6,  5:25pm, m0viefreak wrote:
} Subject: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks
}
} Using PAT_STATIC, any of the functions called in the hook that make
} use of patterns, implicitly change the pattern we use in the
} isearch loop. In the best case this results in pattern-not-found,
} in the worst case we get a dump.

This indicates that PAT_STATIC is unsafe anywhere signal queuing is
NOT in effect, because a trap handler could invoke pattern matching
in the same way.

PAT_STATIC already causes the compiled pattern to be placed on the heap,
so we could introduce a PAT_HEAPDUP that works like PAT_ZDUP except
there would be no need to explicity freepatprog().  Or we could look
for uses of PAT_STATIC and wrap them in queue_signals(), if the scope
is sufficiently narrow.

Either way we have to track down and examine every use of PAT_STATIC,
so ...

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Peter Stephenson]

On Sun, 8 Jan 2017 11:33:58 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> PAT_STATIC already causes the compiled pattern to be placed on the heap,
> so we could introduce a PAT_HEAPDUP that works like PAT_ZDUP except
> there would be no need to explicity freepatprog().

That sounds like a good idea.

pws

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Daniel Shahaf]

m0viefreak wrote on Fri, Jan 06, 2017 at 17:25:41 +0000:
> Minimal reproducing example:
> 
> % bindkey '^R' history-incremental-pattern-search-backward
> % evil_hook() { a=(); : ${a[(r)foo*]}; };
> % zle -N zle-isearch-update evil_hook
> % : foo
> % : bar
> % : baz
> %
> 
> type: <^R>b
> % : baz
> bck-i-search: b_
> 
> type: <^R>
> % : foo
> bck-i-search: b_
> 
> ': foo' is found instead of ': bar' because evil_hook modified the
> static pattern used in isearch.
> 
> Related: zsh-syntax-highlighting issue which found this bug:
> https://github.com/zsh-users/zsh-syntax-highlighting/issues/407

Is there a way to probe for this bug at runtime?  I.e., a way for
a script to determine whether the zsh binary that interprets it has or
hasn't this bug?

We need this in z-sy-h to choose between two codepaths (and we'd
rather not use is-at-least in case distros backport m0vie's patch).

Thanks,

Daniel

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Sun, Jan 8, 2017 at 5:35 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Is there a way to probe for this bug at runtime?  I.e., a way for
> a script to determine whether the zsh binary that interprets it has or
> hasn't this bug?

You'd need something similar to the zpty-based Test/ scripts for
completion and vicmd, because you need to invoke interactive search
with a hook function.

For this specific bug, that is.  One could probably devise by
examination of all uses of PAT_STATIC in the code base a test for the
more general case with trap handlers that I mentioned elsewhere in the
thread.

Fwd: Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

Primenet vs. Gmail again.

---------- Forwarded message ----------
From: "Bart Schaefer" <schaefer@brasslantern.com>
Date: Jan 8, 2017 5:48 PM
Subject: Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks
To: "zsh-workers@zsh.org" <zsh-workers@zsh.org>
Cc:

On Sun, Jan 8, 2017 at 5:35 PM, Daniel Shahaf <d.s@daniel.shahaf.name>
wrote:
>
> Is there a way to probe for this bug at runtime?  I.e., a way for
> a script to determine whether the zsh binary that interprets it has or
> hasn't this bug?

You'd need something similar to the zpty-based Test/ scripts for
completion and vicmd, because you need to invoke interactive search
with a hook function.

For this specific bug, that is.  One could probably devise by
examination of all uses of PAT_STATIC in the code base a test for the
more general case with trap handlers that I mentioned elsewhere in the
thread.

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Sun, 8 Jan 2017, Peter Stephenson wrote:

> On Sun, 8 Jan 2017 11:33:58 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > PAT_STATIC already causes the compiled pattern to be placed on the heap,
> > so we could introduce a PAT_HEAPDUP that works like PAT_ZDUP except
> > there would be no need to explicity freepatprog().
>
> That sounds like a good idea.

Of course I had this wrong -- the pattern is by default in a never-freed
block of zalloc'd memory used by the pattern compiler, and PAT_STATIC
causes it to remain there.  (I misread a "!" in a conditional.)

So PAT_HEAPDUP is already there, it's just (not PAT_STATIC) ... and we
are back to deciding whether each individual use of PAT_STATIC is safe
WRT signals and hooks.

Should we make it explicit anyway?

diff --git a/Src/zsh.h b/Src/zsh.h
index d022260..c387414 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1531,6 +1531,7 @@ struct patstralloc {

 /* Flags used in pattern matchers (Patprog) and passed down to patcompile */

+#define PAT_HEAPDUP	0x0000	/* Dummy flag for default behavior */
 #define PAT_FILE	0x0001	/* Pattern is a file name */
 #define PAT_FILET	0x0002	/* Pattern is top level file, affects ~ */
 #define PAT_ANY		0x0004	   /* Match anything (cheap "*") */

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Peter Stephenson]

On Sat, 28 Jan 2017 11:02:24 -0800 (PST)
Bart Schaefer <schaefer@brasslantern.com> wrote:
> So PAT_HEAPDUP is already there, it's just (not PAT_STATIC) ... and we
> are back to deciding whether each individual use of PAT_STATIC is safe
> WRT signals and hooks.
> 
> Should we make it explicit anyway?
> 
> diff --git a/Src/zsh.h b/Src/zsh.h
> index d022260..c387414 100644
> --- a/Src/zsh.h
> +++ b/Src/zsh.h
> @@ -1531,6 +1531,7 @@ struct patstralloc {
> 
>  /* Flags used in pattern matchers (Patprog) and passed down to patcompile */
> 
> +#define PAT_HEAPDUP	0x0000	/* Dummy flag for default behavior */

It's ages since I looked at this.  Yes, that's definitely clearer.

pws

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Sat, 28 Jan 2017, Bart Schaefer wrote:

> ... back to deciding whether each individual use of PAT_STATIC is safe
> WRT signals and hooks.

Crimony, it looks like pretty much all of compctl.c is signal-UNsafe.

All the functions are static[*] except for the zmodload entry points.
Would it be asking for trouble to wrap all the builtin and widget
implementations of compctl in queue_signals()/unqueue_signals()?


[*] Or could be but for no apparent reason are not.

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Peter Stephenson]

On Sat, 28 Jan 2017 11:39:02 -0800 (PST)
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sat, 28 Jan 2017, Bart Schaefer wrote:
> 
> > ... back to deciding whether each individual use of PAT_STATIC is safe
> > WRT signals and hooks.
> 
> Crimony, it looks like pretty much all of compctl.c is signal-UNsafe.
> 
> All the functions are static[*] except for the zmodload entry points.
> Would it be asking for trouble to wrap all the builtin and widget
> implementations of compctl in queue_signals()/unqueue_signals()?

Well, I think the definitive answer is probably the usual "who knows
what anyone's doing with zsh features?"

But my gut feel is compctl only ever got to the point that it was doing
simple local stuff that finished quickly, and we're morally entitled to
assume that anything that isn't like that is using compsys.  So I'd be
inclined to try what you suggest and just make sure a few simple builtin
completions with compctl work.

pws

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Jan 28,  7:47pm, Peter Stephenson wrote:
}
} But my gut feel is compctl only ever got to the point that it was doing
} simple local stuff that finished quickly, and we're morally entitled to
} assume that anything that isn't like that is using compsys.  So I'd be
} inclined to try what you suggest and just make sure a few simple builtin
} completions with compctl work.

OK, with that confirmed ... here's a largish patch.

It's somewhat arbitrary whether to change the PAT_* flag or to apply
signal queuing.  For example the patch that started all this (40285)
uses PAT_ZDUP, but it would be possible to allow the pattern to be
placed on the heap every time instead.  However this is in a loop,
and the pattern may change multiple times which could lead to several
patterns piling up on the heap before we finally finish the loop and
clean up.  So I elected not to leave that as it was, though I updated
some comments.

In other cases it was obvious that global state was being changed so
it seemed better to add, or to widen the scope of, queue_signals().



diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 0ef7539..2c87be1 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -544,7 +544,8 @@ ptyread(char *nam, Ptycmd cmd, char **args, int noblock, int mustmatch)
 	p = dupstring(args[1]);
 	tokenize(p);
 	remnulargs(p);
-	if (!(prog = patcompile(p, PAT_STATIC, NULL))) {
+	/* Signals handlers might stomp PAT_STATIC */
+	if (!(prog = patcompile(p, PAT_ZDUP, NULL))) {
 	    zwarnnam(nam, "bad pattern: %s", args[1]);
 	    return 1;
 	}
@@ -682,9 +683,14 @@ ptyread(char *nam, Ptycmd cmd, char **args, int noblock, int mustmatch)
 	write_loop(1, buf, used);
     }
 
-    if (seen && (!prog || matchok || !mustmatch))
-	return 0;
-    return cmd->fin + 1;
+    {
+	int ret = cmd->fin + 1;
+	if (seen && (!prog || matchok || !mustmatch))
+	    ret = 0;
+	if (prog)
+	    freepatprog(prog);
+	return ret;
+    }
 }
 
 static int
diff --git a/Src/Modules/zutil.c b/Src/Modules/zutil.c
index d95c0c2..19a8306 100644
--- a/Src/Modules/zutil.c
+++ b/Src/Modules/zutil.c
@@ -510,25 +510,33 @@ bin_zstyle(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    zwarnnam(nam, "too many arguments");
 	    return 1;
 	}
+
+	queue_signals();	/* Protect PAT_STATIC */
+
 	if (context) {
 	    tokenize(context);
 	    zstyle_contprog = patcompile(context, PAT_STATIC, NULL);
 
-	    if (!zstyle_contprog)
+	    if (!zstyle_contprog) {
+		unqueue_signals();
 		return 1;
+	    }
 	} else
 	    zstyle_contprog = NULL;
 
 	if (stylename) {
 	    s = (Style)zstyletab->getnode2(zstyletab, stylename);
-	    if (!s)
+	    if (!s) {
+		unqueue_signals();
 		return 1;
+	    }
 	    zstyletab->printnode(&s->node, list);
 	} else {
 	    scanhashtable(zstyletab, 1, 0, 0,
 			  zstyletab->printnode, list);
 	}
 
+	unqueue_signals();
 	return 0;
     }
     switch (args[0][1]) {
@@ -675,14 +683,20 @@ bin_zstyle(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    char **vals;
 	    Patprog prog;
 
+	    queue_signals();	/* Protect PAT_STATIC */
+
 	    tokenize(args[3]);
 
 	    if ((vals = lookupstyle(args[1], args[2])) &&
 		(prog = patcompile(args[3], PAT_STATIC, NULL))) {
 		while (*vals)
-		    if (pattry(prog, *vals++))
+		    if (pattry(prog, *vals++)) {
+			unqueue_signals();
 			return 0;
+		    }
 	    }
+
+	    unqueue_signals();
 	    return 1;
 	}
 	break;
diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
index 52c6f12..9e6ccb4 100644
--- a/Src/Zle/compctl.c
+++ b/Src/Zle/compctl.c
@@ -99,7 +99,7 @@ freecompctlp(HashNode hn)
 }
 
 /**/
-void
+static void
 freecompctl(Compctl cc)
 {
     if (cc == &cc_default ||
@@ -142,7 +142,7 @@ freecompctl(Compctl cc)
 }
 
 /**/
-void
+static void
 freecompcond(void *a)
 {
     Compcond cc = (Compcond) a;
@@ -186,7 +186,7 @@ freecompcond(void *a)
 }
 
 /**/
-int
+static int
 compctlread(char *name, char **args, Options ops, char *reply)
 {
     char *buf, *bptr;
@@ -1564,6 +1564,8 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
     Compctl cc = NULL;
     int ret = 0;
 
+    queue_signals();
+
     /* clear static flags */
     cclist = 0;
     showmask = 0;
@@ -1571,12 +1573,15 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
     /* Parse all the arguments */
     if (*argv) {
 	/* Let's see if this is a global matcher definition. */
-	if ((ret = get_gmatcher(name, argv)))
+	if ((ret = get_gmatcher(name, argv))) {
+	    unqueue_signals();
 	    return ret - 1;
+	}
 
 	cc = (Compctl) zshcalloc(sizeof(*cc));
 	if (get_compctl(name, &argv, cc, 1, 0, 0)) {
 	    freecompctl(cc);
+	    unqueue_signals();
 	    return 1;
 	}
 
@@ -1604,6 +1609,7 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
 	printcompctl((cclist & COMP_LIST) ? "" : "DEFAULT", &cc_default, 0, 0);
  	printcompctl((cclist & COMP_LIST) ? "" : "FIRST", &cc_first, 0, 0);
 	print_gmatcher((cclist & COMP_LIST));
+	unqueue_signals();
 	return ret;
     }
 
@@ -1642,6 +1648,7 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
 	    printcompctl("", &cc_first, 0, 0);
 	if (cclist & COMP_LISTMATCH)
 	    print_gmatcher(COMP_LIST);
+	unqueue_signals();
 	return ret;
     }
 
@@ -1656,6 +1663,7 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
 	    compctl_process_cc(argv, cc);
     }
 
+    unqueue_signals();
     return ret;
 }
 
@@ -1667,12 +1675,18 @@ bin_compctl(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
 static int
 bin_compcall(char *name, UNUSED(char **argv), Options ops, UNUSED(int func))
 {
+    int ret;
+
     if (incompfunc != 1) {
 	zwarnnam(name, "can only be called from completion function");
 	return 1;
     }
-    return makecomplistctl((OPT_ISSET(ops,'T') ? 0 : CFN_FIRST) |
-			   (OPT_ISSET(ops,'D') ? 0 : CFN_DEFAULT));
+
+    queue_signals();
+    ret = makecomplistctl((OPT_ISSET(ops,'T') ? 0 : CFN_FIRST) |
+			  (OPT_ISSET(ops,'D') ? 0 : CFN_DEFAULT));
+    unqueue_signals();
+    return ret;
 }
 
 /*
@@ -1756,6 +1770,8 @@ ccmakehookfn(UNUSED(Hookdef dummy), struct ccmakedat *dat)
     int onm = nmatches, odm = diffmatches, osi = movefd(0);
     LinkNode n;
 
+    queue_signals();
+
     /* We build a copy of the list of matchers to use to make sure that this
      * works even if a shell function called from the completion code changes
      * the global matchers. */
@@ -1883,6 +1899,8 @@ ccmakehookfn(UNUSED(Hookdef dummy), struct ccmakedat *dat)
     }
     redup(osi, 0);
     dat->lst = 1;
+
+    unqueue_signals();
     return 0;
 }
 
@@ -2044,7 +2062,7 @@ maketildelist(void)
 /* This does the check for compctl -x `n' and `N' patterns. */
 
 /**/
-int
+static int
 getcpat(char *str, int cpatindex, char *cpat, int class)
 {
     char *s, *t, *p;
diff --git a/Src/Zle/complete.c b/Src/Zle/complete.c
index 48fcd47..49b338f 100644
--- a/Src/Zle/complete.c
+++ b/Src/Zle/complete.c
@@ -896,6 +896,8 @@ do_comp_vars(int test, int na, char *sa, int nb, char *sb, int mod)
 	    int i, l = arrlen(compwords), t = 0, b = 0, e = l - 1;
 	    Patprog pp;
 
+	    queue_signals();	/* Protect PAT_STATIC */
+
 	    i = compcurrent - 1;
 	    if (i < 0 || i >= l)
 		return 0;
@@ -930,6 +932,9 @@ do_comp_vars(int test, int na, char *sa, int nb, char *sb, int mod)
 		t = 0;
 	    if (t && mod)
 		restrict_range(b, e);
+
+	    unqueue_signals();
+
 	    return t;
 	}
     case CVT_PRENUM:
@@ -952,6 +957,8 @@ do_comp_vars(int test, int na, char *sa, int nb, char *sb, int mod)
 	{
 	    Patprog pp;
 
+	    queue_signals();	/* Protect PAT_STATIC */
+
 	    if (!na)
 		return 0;
 
@@ -1036,6 +1043,9 @@ do_comp_vars(int test, int na, char *sa, int nb, char *sb, int mod)
 		if (mod)
 		    ignore_suffix(ol - (p - compsuffix));
 	    }
+
+	    unqueue_signals();
+
 	    return 1;
 	}
     }
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 5b9ceec..325da6d 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -3928,6 +3928,8 @@ bin_comptry(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 		    if (*q) {
 			char *qq, *qqq;
 
+			queue_signals();
+
 			if (c)
 			    *c = '\0';
 
@@ -3999,6 +4001,8 @@ bin_comptry(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 			}
 			if (c)
 			    *c = ':';
+
+			unqueue_signals();
 		    }
 		}
 		if (num) {
@@ -4708,6 +4712,8 @@ cfp_add_sdirs(LinkList final, LinkList orig, char *skipped,
 		if (!*p)
 		    continue;
 
+		queue_signals();	/* Protect PAT_STATIC */
+
 		tokenize(f);
 		pprog = patcompile(f, PAT_STATIC, NULL);
 		untokenize(f);
@@ -4740,6 +4746,8 @@ cfp_add_sdirs(LinkList final, LinkList orig, char *skipped,
 			}
 		    }
 		}
+
+		unqueue_signals();
 	    }
 	}
     }
diff --git a/Src/Zle/zle_hist.c b/Src/Zle/zle_hist.c
index 434735d..581ca49 100644
--- a/Src/Zle/zle_hist.c
+++ b/Src/Zle/zle_hist.c
@@ -1220,10 +1220,12 @@ doisearch(char **args, int dir, int pattern)
 		char *patbuf = ztrdup(sbuf);
 		char *patstring;
 		/*
-		 * Do not use static pattern buffer (PAT_STATIC) since we call zle hooks,
-		 * which might call other pattern functions. Use PAT_ZDUP instead.
-		 * Use PAT_NOANCH because we don't need the match
-		 * anchored to the end, even if it is at the start.
+		 * Do not use static pattern buffer (PAT_STATIC) since we
+		 * call zle hooks, which might call other pattern
+		 * functions.  Use PAT_ZDUP because we re-use the pattern
+		 * in subsequent loops, so we can't pushheap/popheap.
+		 * Use PAT_NOANCH because we don't need the match anchored
+		 * to the end, even if it is at the start.
 		 */
 		int patflags = PAT_ZDUP|PAT_NOANCH;
 		if (sbuf[0] == '^') {
diff --git a/Src/builtin.c b/Src/builtin.c
index 219fbc9..394d206 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -539,18 +539,18 @@ bin_enable(char *name, char **argv, Options ops, int func)
     /* With -m option, treat arguments as glob patterns. */
     if (OPT_ISSET(ops,'m')) {
 	for (; *argv; argv++) {
+	    queue_signals();
+
 	    /* parse pattern */
 	    tokenize(*argv);
-	    if ((pprog = patcompile(*argv, PAT_STATIC, 0))) {
-		queue_signals();
+	    if ((pprog = patcompile(*argv, PAT_STATIC, 0)))
 		match += scanmatchtable(ht, pprog, 0, 0, 0, scanfunc, 0);
-		unqueue_signals();
-	    }
 	    else {
 		untokenize(*argv);
 		zwarnnam(name, "bad pattern : %s", *argv);
 		returnval = 1;
 	    }
+	    unqueue_signals();
 	}
 	/* If we didn't match anything, we return 1. */
 	if (!match)
@@ -3136,9 +3136,9 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	} else if (OPT_ISSET(ops,'m')) {
 	    /* List matching functions. */
 	    for (; *argv; argv++) {
+		queue_signals();
 		tokenize(*argv);
 		if ((pprog = patcompile(*argv, PAT_STATIC, 0))) {
-		    queue_signals();
 		    for (p = mathfuncs, q = NULL; p; q = p) {
 			MathFunc next;
 			do {
@@ -3157,12 +3157,12 @@ bin_functions(char *name, char **argv, Options ops, int func)
 			if (p)
 			    p = p->next;
 		    }
-		    unqueue_signals();
 		} else {
 		    untokenize(*argv);
 		    zwarnnam(name, "bad pattern : %s", *argv);
 		    returnval = 1;
 		}
+		unqueue_signals();
 	    }
 	} else if (OPT_PLUS(ops,'M')) {
 	    /* Delete functions. -m is allowed but is handled above. */
@@ -3312,11 +3312,11 @@ bin_functions(char *name, char **argv, Options ops, int func)
     if (OPT_ISSET(ops,'m')) {
 	on &= ~PM_UNDEFINED;
 	for (; *argv; argv++) {
+	    queue_signals();
 	    /* expand argument */
 	    tokenize(*argv);
 	    if ((pprog = patcompile(*argv, PAT_STATIC, 0))) {
 		/* with no options, just print all functions matching the glob pattern */
-		queue_signals();
 		if (!(on|off) && !OPT_ISSET(ops,'X')) {
 		    scanmatchshfunc(pprog, 1, 0, DISABLED,
 				   shfunctab->printnode, pflags, expand);
@@ -3336,12 +3336,12 @@ bin_functions(char *name, char **argv, Options ops, int func)
 			    }
 		    }
 		}
-		unqueue_signals();
 	    } else {
 		untokenize(*argv);
 		zwarnnam(name, "bad pattern : %s", *argv);
 		returnval = 1;
 	    }
+	    unqueue_signals();
 	}
 	return returnval;
     }
@@ -3469,11 +3469,11 @@ bin_unset(char *name, char **argv, Options ops, int func)
     /* with -m option, treat arguments as glob patterns */
     if (OPT_ISSET(ops,'m')) {
 	while ((s = *argv++)) {
+	    queue_signals();
 	    /* expand */
 	    tokenize(s);
 	    if ((pprog = patcompile(s, PAT_STATIC, NULL))) {
 		/* Go through the parameter table, and unset any matches */
-		queue_signals();
 		for (i = 0; i < paramtab->hsize; i++) {
 		    for (pm = (Param) paramtab->nodes[i]; pm; pm = next) {
 			/* record pointer to next, since we may free this one */
@@ -3486,12 +3486,12 @@ bin_unset(char *name, char **argv, Options ops, int func)
 			}
 		    }
 		}
-		unqueue_signals();
 	    } else {
 		untokenize(s);
 		zwarnnam(name, "bad pattern : %s", s);
 		returnval = 1;
 	    }
+	    unqueue_signals();
 	}
 	/* If we didn't match anything, we return 1. */
 	if (!match)
@@ -3655,6 +3655,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	    pushheap();
 	    matchednodes = newlinklist();
 	}
+	queue_signals();
 	for (; *argv; argv++) {
 	    /* parse the pattern */
 	    tokenize(*argv);
@@ -3664,7 +3665,6 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		returnval = 1;
 		continue;
 	    }
-	    queue_signals();
 	    if (!OPT_ISSET(ops,'p')) {
 		/* -p option is for path search only.    *
 		 * We're not using it, so search for ... */
@@ -3695,9 +3695,9 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	    scanmatchtable(cmdnamtab, pprog, 1, 0, 0,
 			   (all ? fetchcmdnamnode : cmdnamtab->printnode),
 			   printflags);
-
-	    unqueue_signals();
+	    run_queued_signals();
 	}
+	unqueue_signals();
 	if (all) {
 	    allmatched = argv = zlinklist2array(matchednodes);
 	    matchednodes = NULL;
@@ -4024,11 +4024,11 @@ bin_unhash(char *name, char **argv, Options ops, int func)
      * "unhash -m '*'" is legal, but not recommended.    */
     if (OPT_ISSET(ops,'m')) {
 	for (; *argv; argv++) {
+	    queue_signals();
 	    /* expand argument */
 	    tokenize(*argv);
 	    if ((pprog = patcompile(*argv, PAT_STATIC, NULL))) {
 		/* remove all nodes matching glob pattern */
-		queue_signals();
 		for (i = 0; i < ht->hsize; i++) {
 		    for (hn = ht->nodes[i]; hn; hn = nhn) {
 			/* record pointer to next, since we may free this one */
@@ -4039,12 +4039,12 @@ bin_unhash(char *name, char **argv, Options ops, int func)
 			}
 		    }
 		}
-		unqueue_signals();
 	    } else {
 		untokenize(*argv);
 		zwarnnam(name, "bad pattern : %s", *argv);
 		returnval = 1;
 	    }
+	    unqueue_signals();
 	}
 	/* If we didn't match anything, we return 1. */
 	if (!match)
@@ -4127,18 +4127,18 @@ bin_alias(char *name, char **argv, Options ops, UNUSED(int func))
      * glob patterns of aliases to display.       */
     if (OPT_ISSET(ops,'m')) {
 	for (; *argv; argv++) {
+	    queue_signals();
 	    tokenize(*argv);  /* expand argument */
 	    if ((pprog = patcompile(*argv, PAT_STATIC, NULL))) {
 		/* display the matching aliases */
-		queue_signals();
 		scanmatchtable(ht, pprog, 1, flags1, flags2,
 			       ht->printnode, printflags);
-		unqueue_signals();
 	    } else {
 		untokenize(*argv);
 		zwarnnam(name, "bad pattern : %s", *argv);
 		returnval = 1;
 	    }
+	    unqueue_signals();
 	}
 	return returnval;
     }
@@ -4348,10 +4348,12 @@ bin_print(char *name, char **args, Options ops, int func)
 	    zwarnnam(name, "no pattern specified");
 	    return 1;
 	}
+	queue_signals();
 	tokenize(*args);
 	if (!(pprog = patcompile(*args, PAT_STATIC, NULL))) {
 	    untokenize(*args);
 	    zwarnnam(name, "bad pattern: %s", *args);
+	    unqueue_signals();
 	    return 1;
 	}
 	for (t = p = ++args; *p; p++)
@@ -4359,6 +4361,7 @@ bin_print(char *name, char **args, Options ops, int func)
 		*t++ = *p;
 	*t = NULL;
 	first = args;
+	unqueue_signals();
 	if (fmt && !*args) return 0;
     }
     /* compute lengths, and interpret according to -P, -D, -e, etc. */
diff --git a/Src/cond.c b/Src/cond.c
index 42e9de3..8ab0193 100644
--- a/Src/cond.c
+++ b/Src/cond.c
@@ -295,6 +295,8 @@ evalcond(Estate state, char *fromtest)
 	    int test, npat = state->pc[1];
 	    Patprog pprog = state->prog->pats[npat];
 
+	    queue_signals();
+
 	    if (pprog == dummy_patprog1 || pprog == dummy_patprog2) {
 		char *opat;
 		int save;
@@ -308,6 +310,7 @@ evalcond(Estate state, char *fromtest)
 		if (!(pprog = patcompile(right, (save ? PAT_ZDUP : PAT_STATIC),
 					 NULL))) {
 		    zwarnnam(fromtest, "bad pattern: %s", right);
+		    unqueue_signals();
 		    return 2;
 		}
 		else if (save)
@@ -316,6 +319,8 @@ evalcond(Estate state, char *fromtest)
 	    state->pc += 2;
 	    test = (pprog && pattry(pprog, left));
 
+	    unqueue_signals();
+
 	    return !(ctype == COND_STRNEQ ? !test : test);
 	}
     case COND_STRLT:
diff --git a/Src/glob.c b/Src/glob.c
index 623e6f1..ff6b258 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -2462,13 +2462,20 @@ xpandbraces(LinkList list, LinkNode *np)
 int
 matchpat(char *a, char *b)
 {
-    Patprog p = patcompile(b, PAT_STATIC, NULL);
+    Patprog p;
+    int ret;
 
-    if (!p) {
+    queue_signals();	/* Protect PAT_STATIC */
+
+    if (!(p = patcompile(b, PAT_STATIC, NULL))) {
 	zerr("bad pattern: %s", b);
-	return 0;
-    }
-    return pattry(p, a);
+	ret = 0;
+    } else
+      ret = pattry(p, a);
+
+    unqueue_signals();
+
+    return ret;
 }
 
 /* do the ${foo%%bar}, ${foo#bar} stuff */
diff --git a/Src/loop.c b/Src/loop.c
index ae87b2f..f7eae30 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -620,7 +620,9 @@ execcase(Estate state, int do_exec)
 	    spprog = state->prog->pats + npat;
 	    pprog = NULL;
 	    pat = NULL;
-	
+
+	    queue_signals();
+
 	    if (isset(XTRACE)) {
 		int htok = 0;
 		pat = dupstring(ecrawstr(state->prog, state->pc, &htok));
@@ -657,6 +659,8 @@ execcase(Estate state, int do_exec)
 		patok = anypatok = 1;
 	    state->pc += 2;
 	    nalts--;
+
+	    unqueue_signals();
 	}
 	state->pc += 2 * nalts;
 	if (isset(XTRACE)) {
diff --git a/Src/options.c b/Src/options.c
index e0b67d2..2b5795b 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -647,7 +647,7 @@ bin_setopt(char *nam, char **args, UNUSED(Options ops), int isun)
 
 	    /* Expand the current arg. */
 	    tokenize(s);
-	    if (!(pprog = patcompile(s, PAT_STATIC, NULL))) {
+	    if (!(pprog = patcompile(s, PAT_HEAPDUP, NULL))) {
 		zwarnnam(nam, "bad pattern: %s", *args);
 		continue;
 	    }
diff --git a/Src/parse.c b/Src/parse.c
index 314cc09..699ea49 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -3413,6 +3413,7 @@ build_cur_dump(char *nam, char *dump, char **names, int match, int map,
 
 	for (; *names; names++) {
 	    tokenize(pat = dupstring(*names));
+	    /* Signal-safe here, caller queues signals */
 	    if (!(pprog = patcompile(pat, PAT_STATIC, NULL))) {
 		zwarnnam(nam, "bad pattern: %s", *names);
 		close(dfd);

Re: [PATCH] isearch: do not use PAT_STATIC since we call zle hooks

[Bart Schaefer]

On Sat, 28 Jan 2017, Bart Schaefer wrote:

> So I elected not to leave that as it was

There's a stray "not" in there, I edited from "not to change" and ...