Colon-array variables can crash "sh" emulation

Colon-array variables can crash "sh" emulation

[Bart Schaefer]

On Apr 14,  7:39pm, Anthony Fletcher wrote:
} 
} Playing further I provoked a segmentation fault! This dies:
}
}    ARGV0=sh zsh -c 'typeset -U path PATH'

Well, well, well.

The special_params array in Src/params.c is divided into two sections,
parameters available in all emulations and those available only in zsh
emulation.

Trouble is the parameters in the first group include numerous colon-array
scalars that are potentially tied to true-arrays from the second group.

This normally doesn't cause any issues, because the parameters in the
second group are initialized only in the appropriate emulation.

BUT -- the parameters in the first group will still try to read values
from the linked parameters in the second group if the attributes of the
first parameter are changed, as by "typeset -U".  Again this is usually
not a problem because the linked parameters are still not set.

However, if the linked parameter has been declared scalar, bin_typeset
will attempt to manipulate it as an array, and wallabies go roaming the
outback, as one of PWS's comments puts it.

This will happen with any pair of variables: path/PATH, watch/WATCH, etc.

However, I'm not sure of the best way to approach fixing this.  My first
thought was that the linkage should be broken during createparamtable(),
but that turns out to be rather ugly to do.

Here's what valgrind has to say:

==16106== Invalid read of size 1
==16106==    at 0x808DCE8: hasher (hashtable.c:85)
==16106==    by 0x808E0BF: gethashnode2 (hashtable.c:255)
==16106==    by 0x80C115F: arrayuniq (params.c:4001)
==16106==    by 0x80C123B: uniqarray (params.c:4031)
==16106==    by 0x80646F7: typeset_single (builtin.c:2201)
==16106==    by 0x806753A: bin_typeset (builtin.c:2893)
==16106==    by 0x805F232: execbuiltin (builtin.c:481)
==16106==    by 0x808066F: execcmd_exec (exec.c:3879)
==16106==    by 0x807AFC3: execpline2 (exec.c:1872)
==16106==    by 0x8079E36: execpline (exec.c:1602)
==16106==    by 0x807945B: execlist (exec.c:1360)
==16106==    by 0x8078C69: execode (exec.c:1141)
==16106==  Address 0x6C616300 is not stack'd, malloc'd or (recently) free'd

Any suggestions?

Re: Colon-array variables can crash "sh" emulation

[Peter Stephenson]

On Fri, 14 Apr 2017 19:31:04 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 14,  7:39pm, Anthony Fletcher wrote:
> } 
> } Playing further I provoked a segmentation fault! This dies:
> }
> }    ARGV0=sh zsh -c 'typeset -U path PATH'
> 
> Well, well, well.
> 
> The special_params array in Src/params.c is divided into two sections,
> parameters available in all emulations and those available only in zsh
> emulation.
> 
> Trouble is the parameters in the first group include numerous colon-array
> scalars that are potentially tied to true-arrays from the second group.
> 
> This normally doesn't cause any issues, because the parameters in the
> second group are initialized only in the appropriate emulation.
> 
> BUT -- the parameters in the first group will still try to read values
> from the linked parameters in the second group if the attributes of the
> first parameter are changed, as by "typeset -U".  Again this is usually
> not a problem because the linked parameters are still not set.
> 
> However, if the linked parameter has been declared scalar, bin_typeset
> will attempt to manipulate it as an array, and wallabies go roaming the
> outback, as one of PWS's comments puts it.
> 
> This will happen with any pair of variables: path/PATH, watch/WATCH, etc.
> 
> However, I'm not sure of the best way to approach fixing this.  My first
> thought was that the linkage should be broken during createparamtable(),
> but that turns out to be rather ugly to do.

As path is not a special at all in this case, the fix must be somewhere
or other in the linkage from PATH to path.

The only robust solution looks like being not let PATH's ename element
point at path.  Otherwise the code following ename has to jump through
hoops to see if the parameters are "really" linked, which is far too
late.

I think we need somehow to keep track of the variables in params.c
defined by IPDEF8 and in sh emulation not set ename or null it out using
the same test that decides if the reference parameters will be special.
The path of least resistance seems to be simply declaring the variables
separately.

The fly in the ointment here is that colonarrsetfn() refuses to fix up
the environment unless the ename element is set.  It's not clear to me
why since I don't think ename can be unset for special tied variables
which are the only use of colonarrsetfn().  If that can go the following
might be good enough.  It needs some more verification using sh emulation
at shell start, though.

pws

diff --git a/Src/params.c b/Src/params.c
index a9683a6..d92dd22 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -361,6 +361,17 @@ IPDEF7("PS3", &prompt3),
 IPDEF7R("PS4", &prompt4),
 IPDEF7("SPROMPT", &sprompt),
 
+#define IPDEF9F(A,B,C,D) {{NULL,A,D|PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(vararray_gsu),0,0,NULL,C,NULL,0}
+#define IPDEF9(A,B,C) IPDEF9F(A,B,C,0)
+IPDEF9F("*", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
+IPDEF9F("@", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
+
+/*
+ * This empty row indicates the end of parameters available in
+ * all emulations.
+ */
+{{NULL,NULL,0},BR(NULL),NULL_GSU,0,0,NULL,NULL,NULL,0},
+
 #define IPDEF8(A,B,C,D) {{NULL,A,D|PM_SCALAR|PM_SPECIAL},BR((void *)B),GSU(colonarr_gsu),0,0,NULL,C,NULL,0}
 IPDEF8("CDPATH", &cdpath, "cdpath", 0),
 IPDEF8("FIGNORE", &fignore, "fignore", 0),
@@ -374,17 +385,6 @@ IPDEF8("ZSH_EVAL_CONTEXT", &zsh_eval_context, "zsh_eval_context", PM_READONLY),
 /* MODULE_PATH is not imported for security reasons */
 IPDEF8("MODULE_PATH", &module_path, "module_path", PM_DONTIMPORT|PM_RESTRICTED),
 
-#define IPDEF9F(A,B,C,D) {{NULL,A,D|PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(vararray_gsu),0,0,NULL,C,NULL,0}
-#define IPDEF9(A,B,C) IPDEF9F(A,B,C,0)
-IPDEF9F("*", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
-IPDEF9F("@", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
-
-/*
- * This empty row indicates the end of parameters available in
- * all emulations.
- */
-{{NULL,NULL,0},BR(NULL),NULL_GSU,0,0,NULL,NULL,NULL,0},
-
 #define IPDEF10(A,B) {{NULL,A,PM_ARRAY|PM_SPECIAL},BR(NULL),GSU(B),10,0,NULL,NULL,NULL,0}
 
 /*
@@ -424,6 +424,26 @@ IPDEF10("pipestatus", pipestatus_gsu),
 };
 
 /*
+ * Alternative versions of colon-separated path parameters for
+ * sh emulation.  These don't link to the array versions.
+ */
+static initparam special_params_sh[] = {
+IPDEF8("CDPATH", &cdpath, NULL, 0),
+IPDEF8("FIGNORE", &fignore, NULL, 0),
+IPDEF8("FPATH", &fpath, NULL, 0),
+IPDEF8("MAILPATH", &mailpath, NULL, 0),
+IPDEF8("WATCH", &watch, NULL, 0),
+IPDEF8("PATH", &path, NULL, PM_RESTRICTED),
+IPDEF8("PSVAR", &psvar, NULL, 0),
+IPDEF8("ZSH_EVAL_CONTEXT", &zsh_eval_context, NULL, PM_READONLY),
+
+/* MODULE_PATH is not imported for security reasons */
+IPDEF8("MODULE_PATH", &module_path, NULL, PM_DONTIMPORT|PM_RESTRICTED),
+
+{{NULL,NULL,0},BR(NULL),NULL_GSU,0,0,NULL,NULL,NULL,0},
+};
+
+/*
  * Special way of referring to the positional parameters.  Unlike $*
  * and $@, this is not readonly.  This parameter is not directly
  * visible in user space.
@@ -753,9 +773,13 @@ createparamtable(void)
     /* Add the special parameters to the hash table */
     for (ip = special_params; ip->node.nam; ip++)
 	paramtab->addnode(paramtab, ztrdup(ip->node.nam), ip);
-    if (!EMULATION(EMULATE_SH|EMULATE_KSH))
+    if (EMULATION(EMULATE_SH|EMULATE_KSH)) {
+	for (ip = special_params_sh; ip->node.nam; ip++)
+	    paramtab->addnode(paramtab, ztrdup(ip->node.nam), ip);
+    } else {
 	while ((++ip)->node.nam)
 	    paramtab->addnode(paramtab, ztrdup(ip->node.nam), ip);
+    }
 
     argvparam = (Param) &argvparam_pm;
 
@@ -3857,8 +3881,7 @@ colonarrsetfn(Param pm, char *x)
 	*dptr = colonsplit(x, pm->node.flags & PM_UNIQUE);
     else
 	*dptr = mkarray(NULL);
-    if (pm->ename)
-	arrfixenv(pm->node.nam, *dptr);
+    arrfixenv(pm->node.nam, *dptr);
     zsfree(x);
 }
 

Re: Colon-array variables can crash "sh" emulation

[Bart Schaefer]

On Apr 20, 11:21am, Peter Stephenson wrote:
}
} The only robust solution looks like being not let PATH's ename element
} point at path.  Otherwise the code following ename has to jump through
} hoops to see if the parameters are "really" linked, which is far too
} late.

Yes.

} The fly in the ointment here is that colonarrsetfn() refuses to fix up
} the environment unless the ename element is set.  It's not clear to me
} why since I don't think ename can be unset for special tied variables
} which are the only use of colonarrsetfn().

This is probably not be true any longer, but at one time I think "ename"
was doing double duty as a flag that the parameter was in fact exported.
My understanding was that it's called "ename" because it's the name in
the environment to which the parameter refers.  This is probably only
the case for the array version of the parameter and not for the scalar,
or something like that.

Re: Colon-array variables can crash "sh" emulation

[Peter Stephenson]

On Thu, 20 Apr 2017 07:56:20 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 20, 11:21am, Peter Stephenson wrote:
> } The fly in the ointment here is that colonarrsetfn() refuses to fix up
> } the environment unless the ename element is set.  It's not clear to me
> } why since I don't think ename can be unset for special tied variables
> } which are the only use of colonarrsetfn().
> 
> This is probably not be true any longer, but at one time I think "ename"
> was doing double duty as a flag that the parameter was in fact exported.

Yes, this has changed significantly since the early days.

The only remaining use of ename I could see that made me scratch my head
was the test just before the export_param() call at the end of
assignstrvalue(), but I think the worst impact is a superfluous call to
addenv().

pws