[BUG] Strange auto-load behaviour when function name contains hyphen

[BUG] Strange auto-load behaviour when function name contains hyphen

[dana]

Hey again. :/

The documentation states that function files intended for zsh-style auto-loading
are allowed to contain a 'simple definition' (`foo() { ... }`) if that's the
only thing in the file. This seems to work fine... except when the function name
contains a hyphen. Illustration:

  % fpath=( . $fpath )

  % print -r 'foo_bar() print foo_bar' > foo_bar
  % autoload -Uz foo_bar
  % foo_bar
  foo_bar

  % print -r 'foo-bar() print foo-bar' > foo-bar
  % autoload -Uz foo-bar
  % foo-bar # No output — function is only defined now
  % foo-bar # Second call actually executes the function
  foo-bar

In the foo-bar case, stripkshdef() doesn't strip the definition out because the
Eprog->strs value for the function contains weird characters (and thus fails the
strcmp() against the expected name). Specifically, it seems the strs value
contains \x9b in place of the hyphens.

Not sure what the significance of that is; that's about as far as i could follow
it, unfortunately.

dana

Re: [BUG] Strange auto-load behaviour when function name contains hyphen

[Peter Stephenson]

On Thu, 14 Dec 2017 00:46:29 -0600
dana <dana@dana.is> wrote:
> The documentation states that function files intended for zsh-style
> auto-loading are allowed to contain a 'simple definition' (`foo() {
> ... }`) if that's the only thing in the file. This seems to work
> fine... except when the function name contains a hyphen.

A while back we had to turn "-" into a token to fix a compatibility
issue with pattern matching in POSIX shells.  This has caused problems
to come out of the wardrobe ever since.  ("Woodwork" implies a level
of solid material without the appropriate magical connotations.)

This has got line offsets including the function line numbering change as it
currently stands but I won't commit that.

pws

diff --git a/Src/exec.c b/Src/exec.c
index fc6d02d..5149349 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5932,6 +5934,7 @@ stripkshdef(Eprog prog, char *name)
 {
     Wordcode pc;
     wordcode code;
+    char *ptr1, *ptr2;
 
     if (!prog)
 	return NULL;
@@ -5942,8 +5945,23 @@ stripkshdef(Eprog prog, char *name)
 	return prog;
     pc++;
     code = *pc++;
-    if (wc_code(code) != WC_FUNCDEF ||
-	*pc != 1 || strcmp(name, ecrawstr(prog, pc + 1, NULL)))
+    if (wc_code(code) != WC_FUNCDEF ||	*pc != 1)
+	return prog;
+
+    /*
+     * See if name of function requested (name) is same as
+     * name of function in word code.  name may still have "-"
+     * tokenised.
+     */
+    ptr1 = name;
+    ptr2 = ecrawstr(prog, pc + 1, NULL);
+    while (*ptr1 && *ptr2) {
+	if (*ptr1 != *ptr2 && *ptr1 != Dash && *ptr1 != '-')
+	    break;
+	ptr1++;
+	ptr2++;
+    }
+    if (*ptr1 || *ptr2)
 	return prog;
 
     {

Re: [BUG] Strange auto-load behaviour when function name contains hyphen

[Peter Stephenson]

On Thu, 14 Dec 2017 10:01:42 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> +	if (*ptr1 != *ptr2 && *ptr1 != Dash && *ptr1 != '-')

Bart noticed the error here.  I stared at for ages without noticing.

pws

diff --git a/Src/exec.c b/Src/exec.c
index fc6d02d..eda1015 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5669,11 +5669,13 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	funcsave->fstack.caller = funcstack ? funcstack->name :
 	    dupstring(funcsave->argv0 ? funcsave->argv0 : argzero);
 	funcsave->fstack.lineno = lineno;
+	funcsave->fstack.flineno = shfunc->lineno;
+	if (funcstack)
+	    funcsave->fstack.flineno += funcstack->flineno;
 	funcsave->fstack.prev = funcstack;
 	funcsave->fstack.tp = FS_FUNC;
 	funcstack = &funcsave->fstack;
 
-	funcsave->fstack.flineno = shfunc->lineno;
 	funcsave->fstack.filename = getshfuncfile(shfunc);
 
 	prog = shfunc->funcdef;
@@ -5932,6 +5934,7 @@ stripkshdef(Eprog prog, char *name)
 {
     Wordcode pc;
     wordcode code;
+    char *ptr1, *ptr2;
 
     if (!prog)
 	return NULL;
@@ -5942,8 +5945,23 @@ stripkshdef(Eprog prog, char *name)
 	return prog;
     pc++;
     code = *pc++;
-    if (wc_code(code) != WC_FUNCDEF ||
-	*pc != 1 || strcmp(name, ecrawstr(prog, pc + 1, NULL)))
+    if (wc_code(code) != WC_FUNCDEF ||	*pc != 1)
+	return prog;
+
+    /*
+     * See if name of function requested (name) is same as
+     * name of function in word code.  name may still have "-"
+     * tokenised.
+     */
+    ptr1 = name;
+    ptr2 = ecrawstr(prog, pc + 1, NULL);
+    while (*ptr1 && *ptr2) {
+	if (*ptr1 != *ptr2 && *ptr1 != Dash && *ptr2 != '-')
+	    break;
+	ptr1++;
+	ptr2++;
+    }
+    if (*ptr1 || *ptr2)
 	return prog;
 
     {

Re: [BUG] Strange auto-load behaviour when function name contains hyphen

[dana]

On 14 Dec 2017, at 04:28, Peter Stephenson <p.stephenson@samsung.com> wrote:
>+    /*
>+     * See if name of function requested (name) is same as
>+     * name of function in word code.  name may still have "-"
>+     * tokenised.
>+     */
>+    ptr1 = name;
>+    ptr2 = ecrawstr(prog, pc + 1, NULL);
>+    while (*ptr1 && *ptr2) {
>+	if (*ptr1 != *ptr2 && *ptr1 != Dash && *ptr2 != '-')

I think there's another error here. name isn't the one that might be tokenised
(or it's not in my case anyway) — it's the other one. (And just swapping the
last two tests would make it so that it accepts a definition with a different
name than expected.)

The patch seems to work for me if i change the condition to this:

  if (*ptr1 != *ptr2 && !(*ptr1 == '-' && *ptr2 == Dash))

Thanks!

dana

Re: [BUG] Strange auto-load behaviour when function name contains hyphen

[dana]

On 14 Dec 2017, at 11:21, dana <dana@dana.is> wrote:
>The patch seems to work for me if i change the condition to this:

Understanding a bit more about the token thing, i went and tested some of the
others defined in zsh.h. The same problem occurs if you have a function name
containing almost any of them. Also, if your function name is quoted or escaped,
the tokens for those meta-characters are present and have to be dealt with for
the comparison too.

I'm not sure why someone would use an asterisk or whatever in a function name,
but i can't find anything in the documentation that says you shouldn't (and i'm
not aware of any issues doing so aside from this).

Should it perform a more comprehensive check? Something like the following?
Possibly it could be optimised to avoid going through the whole dance when it
doesn't need to (considering it's an edge case)...

dana


diff --git a/Src/exec.c b/Src/exec.c
index fc6d02dc3..4c096fd12 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5932,6 +5932,7 @@ stripkshdef(Eprog prog, char *name)
 {
     Wordcode pc;
     wordcode code;
+    char *pname, *litpname;
 
     if (!prog)
 	return NULL;
@@ -5942,10 +5943,25 @@ stripkshdef(Eprog prog, char *name)
 	return prog;
     pc++;
     code = *pc++;
-    if (wc_code(code) != WC_FUNCDEF ||
-	*pc != 1 || strcmp(name, ecrawstr(prog, pc + 1, NULL)))
+    if (wc_code(code) != WC_FUNCDEF || *pc != 1)
 	return prog;
 
+    /*
+     * Detokenise and literalise the name in the word code (it may be quoted)
+     * and make sure it matches the expected name
+     */
+    pname = ecrawstr(prog, pc + 1, NULL);
+    litpname = ztrdup(pname);
+    parse_subst_string(litpname);
+    remnulargs(litpname);
+    untokenize(litpname);
+
+    if (strcmp(name, litpname)) {
+	free(litpname);
+	return prog;
+    }
+    free(litpname);
+
     {
 	Eprog ret;
 	Wordcode end = pc + WC_FUNCDEF_SKIP(code);
diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index 0c00a0477..cd29f658c 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -515,6 +515,22 @@
 0:autoload with absolute path not cancelled by bare autoload
 >I have been loaded by explicit path.
 
+  (
+    fpath=(.)
+    print -n '['
+    for f in ${(s<>):-'x "#$^*$=|`<>?~,-!'}; do
+      # Test with both backslashes and single-quotes
+      print -r -- "${(q)f}_1() print -nr -- ${(q)f}" > ${f}_1
+      print -r -- "${(qq)f}_2() print -nr -- ${(qq)f}" > ${f}_2
+      autoload -Uz -- ${f}_1 ${f}_2
+      ${f}_1
+      ${f}_2
+    done
+    print ']'
+  )
+0:autoload with weird function names and ksh-style definitions
+>[xx  ""##$$^^**$$==||``<<>>??~~,,--!!]
+
   (
     FUNCNEST=0
     fn() { true; }

Re: [BUG] Strange auto-load behaviour when function name contains hyphen

[Peter Stephenson]

On Thu, 14 Dec 2017 11:21:21 -0600
dana <dana@dana.is> wrote:
> On 14 Dec 2017, at 04:28, Peter Stephenson <p.stephenson@samsung.com> wrote:
> >+    /*
> >+     * See if name of function requested (name) is same as
> >+     * name of function in word code.  name may still have "-"
> >+     * tokenised.
> >+     */
> >+    ptr1 = name;
> >+    ptr2 = ecrawstr(prog, pc + 1, NULL);
> >+    while (*ptr1 && *ptr2) {
> >+	if (*ptr1 != *ptr2 && *ptr1 != Dash && *ptr2 != '-')
> 
> I think there's another error here. name isn't the one that might be tokenised
> (or it's not in my case anyway) — it's the other one.

That suggests another bug since I don't think function names in the word
code ought to be tokenised, but quite possibly that hasn't been an issue
up to now.  I haven't gone looking for that --- I've just handled both
cases (and added a test for the obvious case).

> Understanding a bit more about the token thing, i went and tested some of the
> others defined in zsh.h. The same problem occurs if you have a function name
> containing almost any of them.

Dash is a bit of a special case, since in normal zsh syntax it's
actually not a pattern character.  Things like * and ? will cause
immediate expansion, so any function name using them always has to be
quoted.  There's not a lot of point in supporting ths, though, as you
say, it's not explicitly excluded at the moment.

Quotes are stripped from the word on the command line before the
comparision.  I think that's all that sanity really requires.

pws