[PATCH] jp: fix segfaults during parameter expansion

[PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

Running `zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:

> "segmentation fault (core dumped) zsh -fc '(: ${${(PAA)p[foo]}::=x})'

Also happens when testing with machabot:

> 19:42 <jp> > : ${${(PAA)p[foo]}::=x}
> 19:42 <machabot> jp: zsh[248]: segfault at 0 ip b7dfcda3 sp bfeb9ebc
>       error 4 in libc-2.13.so[b7d84000+149000]

Add checks to catch NULL dereferences.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Src/params.c b/Src/params.c
index de7730ae735a44963c..9516185015d878b553 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2016,6 +2016,9 @@ fetchvalue(Value v, char **pptr, int bracks, int flags)
     char sav, c;
     int ppar = 0;
 
+    if (!*pptr)
+	return NULL;
+
     s = t = *pptr;
 
     if (idigit(c = *s)) {
diff --git a/Src/string.c b/Src/string.c
index 9e14ef94919c3e8ec5..7ad8ca7589199e8170 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -144,7 +144,12 @@ dyncat(const char *s1, const char *s2)
 {
     /* This version always uses space from the current heap. */
     char *ptr;
-    size_t l1 = strlen(s1);
+    size_t l1;
+
+    if (!s1 || !s2)
+	return NULL;
+
+    l1 = strlen(s1);
 
     ptr = (char *)zhalloc(l1 + strlen(s2) + 1);
     strcpy(ptr, s1);
diff --git a/Src/subst.c b/Src/subst.c
index d027e3d83cadc631a7..c423bc8433c590a89c 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2577,7 +2577,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
      * the local value system, or we need to get rid of brackets
      * even if there isn't a v.
      */
-    while (v || ((inbrace || (unset(KSHARRAYS) && vunset)) && isbrack(*s))) {
+    while (v || ((inbrace || (unset(KSHARRAYS) && vunset)) && s && isbrack(*s))) {
 	if (!v) {
 	    /*
 	     * Index applied to non-existent parameter; we may or may
@@ -2703,6 +2703,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
      * examine properly later on.
      */
     if (inbrace) {
+	if (!s)
+	    return NULL;
 	c = *s;
 	if (!IS_DASH(c) &&
 	    c != '+' && c != ':' && c != '%'  && c != '/' &&
-- 
2.15.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[Bart Schaefer]

On Sat, Jan 13, 2018 at 10:05 PM, Joey Pabalinas
<joeypabalinas@gmail.com> wrote:
> Running `zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:
>
>> "segmentation fault (core dumped) zsh -fc '(: ${${(PAA)p[foo]}::=x})'
>
> Add checks to catch NULL dereferences.

Thanks for tracking this down.  Defensive programming is always good,
but I think this is indicative of a problem further upstream.

What's the expected output of that substitution?

The following prevents the segfault for me, instead giving the error
"zsh: not an identifier: " (i.e., empty string is not a valid
parameter name).  But perhaps there's a different error that should
occur here if val is NULL?

diff --git a/Src/subst.c b/Src/subst.c
index d027e3d..73491c2 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2430,7 +2430,10 @@ paramsubst(LinkList l, LinkNode n, char **str,
int qt, int pf_flags,
                val = aval[0];
                isarr = 0;
            }
-           s = dyncat(val, s);
+           if (val)
+               s = dyncat(val, s);
+           else
+               s = dupstring(s);
            /* Now behave po-faced as if it was always like that... */
            subexp = 0;
            /*

Re: [PATCH] jp: fix segfaults during parameter expansion

[Bart Schaefer]

On Sun, Jan 14, 2018 at 4:06 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> index d027e3d..73491c2 100644
> --- a/Src/subst.c
> +++ b/Src/subst.c
> @@ -2430,7 +2430,10 @@ paramsubst(LinkList l, LinkNode n, char **str,
> int qt, int pf_flags,

Sorry about the line-wrap there, still standing-in gmail for my regular client.

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

On Sun, Jan 14, 2018 at 04:06:09AM -0800, Bart Schaefer wrote:
>
> Thanks for tracking this down.  Defensive programming is always good,
> but I think this is indicative of a problem further upstream.
> 
> What's the expected output of that substitution?

No problem; to be quite honestly I don't think there is on, haha.
Okdana on irc pointed it out, and I just tracked it down cause
tracking down stuff like this is something I enjoy. When I found the
NULL dereferences it made me wonder "wtf is going on here anyway???" so
I asked:

> 20:30 <jp> also how in the fuck did you even find this
> 20:30 <jp> this is like something a fuzzer would pop out
> 20:30 <okdana> lol
> 20:31 <okdana> i was trying to assign a value to an association element
>       through indirection
> 20:31 <okdana> the AA is nonsense because a single element can't have an
>        array/association assigned to it obv
> 20:32 <okdana> i was just adding and removing stuff randomly to see what
>       would work

so yeah :) Honestly, I mostly just wasn't comfortable with paramater
expansion segfaulting, heh.

On Sun, Jan 14, 2018 at 04:06:09AM -0800, Bart Schaefer wrote:
> 
> The following prevents the segfault for me, instead giving the error
> "zsh: not an identifier: " (i.e., empty string is not a valid
> parameter name).  But perhaps there's a different error that should
> occur here if val is NULL?

That actually helped me realize (imo) a simpler solution; working
on a [PATCH v2] right now.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

Running `zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:

> "segmentation fault (core dumped) zsh -fc ': ${${(PAA)p[foo]}::=x}'

Also happens when testing with machabot:

> 19:42 <jp> > : ${${(PAA)p[foo]}::=x}
> 19:42 <machabot> jp: zsh[248]: segfault at 0 ip b7dfcda3 sp bfeb9ebc
>       error 4 in libc-2.13.so[b7d84000+149000]

Add a simple `dupstring(s2)` fallback instead of pointlessly
trying to concatenate `s2` to NULL and segfaulting.

Also added indication of empty string using `(nil)`; the empty string
case should still provide a somewhat useful error message of

> zsh:1: not an identifier: (nil)

rather than

> zsh:1: not an identifier:

which is fairly confusing.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
Requested-by: Bart Schaefer <schaefer@brasslantern.com>

 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Src/params.c b/Src/params.c
index de7730ae735a44963c..44a942296f23ddf88f 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3004,6 +3004,8 @@ assignsparam(char *s, char *val, int flags)
     int sstart, created = 0;
 
     if (!isident(s)) {
+	if (!strcmp(s, ""))
+	    s = "(nil)";
 	zerr("not an identifier: %s", s);
 	zsfree(val);
 	errflag |= ERRFLAG_ERROR;
diff --git a/Src/string.c b/Src/string.c
index 9e14ef94919c3e8ec5..7c24ab3c45777f31e9 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -126,9 +126,17 @@ mod_export char *
 zhtricat(char const *s1, char const *s2, char const *s3)
 {
     char *ptr;
-    size_t l1 = strlen(s1);
-    size_t l2 = strlen(s2);
+    size_t l1;
+    size_t l2;
 
+    /* String duplicate fallback to prevent NULL derefs */
+    if (!s1 && !s2)
+	return dupstring(s3);
+    if (!s1)
+	l1 = 0, s1 = s2;
+    else
+	l1 = strlen(s1);
+    l2 = strlen(s2);
     ptr = (char *)zhalloc(l1 + l2 + strlen(s3) + 1);
     strcpy(ptr, s1);
     strcpy(ptr + l1, s2);
@@ -144,8 +152,12 @@ dyncat(const char *s1, const char *s2)
 {
     /* This version always uses space from the current heap. */
     char *ptr;
-    size_t l1 = strlen(s1);
+    size_t l1;
 
+    /* String duplicate fallback to prevent NULL derefs */
+    if (!s1)
+	return dupstring(s2);
+    l1 = strlen(s1);
     ptr = (char *)zhalloc(l1 + strlen(s2) + 1);
     strcpy(ptr, s1);
     strcpy(ptr + l1, s2);
@@ -158,8 +170,12 @@ bicat(const char *s1, const char *s2)
 {
     /* This version always uses permanently-allocated space. */
     char *ptr;
-    size_t l1 = strlen(s1);
+    size_t l1;
 
+    /* String duplicate fallback to prevent NULL derefs */
+    if (!s1)
+	return dupstring(s2);
+    l1 = strlen(s1);
     ptr = (char *)zalloc(l1 + strlen(s2) + 1);
     strcpy(ptr, s1);
     strcpy(ptr + l1, s2);
diff --git a/Src/subst.c b/Src/subst.c
index d027e3d83cadc631a7..9a8c635e313687d046 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3150,6 +3150,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 	}
 	if (*itype_end(s, IIDENT, 0)) {
 	    untokenize(s);
+	    if (!strcmp(s, " "))
+		s = "(nil)";
 	    zerr("not an identifier: %s", s);
 	    return NULL;
 	}
@@ -3210,6 +3212,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 	++s;
 	if (*itype_end(s, IIDENT, 0)) {
 	    untokenize(s);
+	    if (!strcmp(s, ""))
+		s = "(nil)";
 	    zerr("not an identifier: %s", s);
 	    return NULL;
 	}
-- 
2.15.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[dana]

On 14 Jan 2018, at 06:06, Bart Schaefer <schaefer@brasslantern.com> wrote:
>What's the expected output of that substitution?

This was my segfault. I didn't expect anything from it exactly, i was just
messing about without giving much thought to what i was doing. But i guess what
i'd originally wanted was a way to indirectly assign a value to an association
element. Seems you have to do something like this?

  local -A assoc
  name='assoc[foo]'
  : ${(P)name::=bar}

dana

Re: [PATCH] jp: fix segfaults during parameter expansion

[Daniel Tameling]

Hi,

while working through my email backlog, I noticed that my zsh didn't
segfault. I used git bisect, and it looks like commit
4b8db48c6bd3c0230a5d81f49e478857adf9cda8 introduced it. Maybe this helps
someone that understands the code base better than me to figure out
what's wrong.

Kind regards
Daniel

Bart Schaefer <schaefer@brasslantern.com> writes:

> On Sat, Jan 13, 2018 at 10:05 PM, Joey Pabalinas
> <joeypabalinas@gmail.com> wrote:
>> Running `zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:
>>
>>> "segmentation fault (core dumped) zsh -fc '(: ${${(PAA)p[foo]}::=x})'
>>
>> Add checks to catch NULL dereferences.
>
> Thanks for tracking this down.  Defensive programming is always good,
> but I think this is indicative of a problem further upstream.
>
> What's the expected output of that substitution?
>
> The following prevents the segfault for me, instead giving the error
> "zsh: not an identifier: " (i.e., empty string is not a valid
> parameter name).  But perhaps there's a different error that should
> occur here if val is NULL?
>
> diff --git a/Src/subst.c b/Src/subst.c
> index d027e3d..73491c2 100644
> --- a/Src/subst.c
> +++ b/Src/subst.c
> @@ -2430,7 +2430,10 @@ paramsubst(LinkList l, LinkNode n, char **str,
> int qt, int pf_flags,
>                 val = aval[0];
>                 isarr = 0;
>             }
> -           s = dyncat(val, s);
> +           if (val)
> +               s = dyncat(val, s);
> +           else
> +               s = dupstring(s);
>             /* Now behave po-faced as if it was always like that... */
>             subexp = 0;
>             /*


-- 
Daniel

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

On Sat, Jan 20, 2018 at 05:16:37PM +0100, Daniel Tameling wrote:
> 
> Hi,
> 
> while working through my email backlog, I noticed that my zsh didn't
> segfault. I used git bisect, and it looks like commit
> 4b8db48c6bd3c0230a5d81f49e478857adf9cda8 introduced it. Maybe this helps
> someone that understands the code base better than me to figure out
> what's wrong.
> 
> Kind regards
> Daniel
> 
> Bart Schaefer <schaefer@brasslantern.com> writes:
> 

Ah, this is actually very helpful; I'll try looking into what this commit
touched later today, thanks!

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[Bart Schaefer]

On Jan 20,  5:16pm, Daniel Tameling wrote:
} 
} while working through my email backlog, I noticed that my zsh didn't
} segfault. I used git bisect, and it looks like commit
} 4b8db48c6bd3c0230a5d81f49e478857adf9cda8 introduced it.

This is a bit of a red herring -- the fault is actually in the handling
of the (P) flag.  What this commit changed was the handling of the (A)
flag, to make it independent of the ${name=value} syntax.

The potential bug in handling of (P) has always been there, but we never
attempted to convert the scalar into an array in that context before.

My patch in workers/42268 addresses the thing that crashes.  However,
I think the following may be better:

torch% : ${${(PAA)p[foo]}::=x}
zsh: parameter name reference used with array


diff --git a/Src/subst.c b/Src/subst.c
index d027e3d..042ef28 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2423,7 +2423,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 	     * substitution.
 	     */
 	    if (isarr) {
-		if (aval[0] && aval[1]) {
+		if (!aval[0] || (aval[0] && aval[1])) {
 		    zerr("parameter name reference used with array");
 		    return NULL;
 		}

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

Running:

> $ zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:
>
> [1] 4441 segmentation fault (core dumped) zsh -fc ': ${${(PAA)p[foo]}::=x}'

Also happens when testing with machabot:

> 19:42 <jp> > : ${${(PAA)p[foo]}::=x}
> 19:42 <machabot> jp: zsh[248]: segfault at 0 ip b7dfcda3 sp bfeb9ebc
>       error 4 in libc-2.13.so[b7d84000+149000]

Add a simple `dupstring(s)` fallback.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
Requested-by: Bart Schaefer <schaefer@brasslantern.com>

 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Src/subst.c b/Src/subst.c
index d027e3d83cadc631a7..d159a92ae1bc1b9c10 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2423,14 +2423,14 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 	     * substitution.
 	     */
 	    if (isarr) {
-		if (aval[0] && aval[1]) {
+		if (!aval[0] || (aval[0] && aval[1])) {
 		    zerr("parameter name reference used with array");
 		    return NULL;
 		}
 		val = aval[0];
 		isarr = 0;
 	    }
-	    s = dyncat(val, s);
+	    s = val ? dyncat(val, s) : dupstring(s);
 	    /* Now behave po-faced as if it was always like that... */
 	    subexp = 0;
 	    /*
-- 
2.16.0

Incorpated your changes, although I feel like the conditional operator is
a better fit here (simpler and much more concise; making each possibility a
lone function call is one of the few uses of `?:` where the intent is
flat-out obvious).

Tested and works great:

> $ : ${${(PAA)p[foo]}::=x}
>
> zsh: parameter name reference used with array

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[Bart Schaefer]

On Jan 20,  3:47pm, Joey Pabalinas wrote:
} 
} Incorpated your changes, although I feel like the conditional operator is
} a better fit here (simpler and much more concise; making each possibility a
} lone function call is one of the few uses of `?:` where the intent is
} flat-out obvious).

There's no reason to do both of these changes.  You do the first one if
you like the "reference used with array" error message, or you do the
second one if you like the "not an identifier" message.  The second
one will never take the dupstring() branch if you have done the first.

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

On Sun, Jan 21, 2018 at 09:43:26AM -0800, Bart Schaefer wrote:
> 
> There's no reason to do both of these changes.  You do the first one if
> you like the "reference used with array" error message, or you do the
> second one if you like the "not an identifier" message.  The second
> one will never take the dupstring() branch if you have done the first.

Noted, I feel like the "not an identifier message", although strange,
is more useful here. That one at least sort of makes sense; the other
"reference used with array" message took a few minutes digging
through the source to even figure out what that even meant
in normal context.

When i send updated patches, what's the best format? A new email after
a separate response mail, or as a single email with the response
at the bottom (or some other third option, maybe)?

Sending this one as its own separate mail for now.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

Running:

> $ zsh -fc ': ${${(PAA)p[foo]}::=x}'` in current zsh versions causes:
>
> [1] 4441 segmentation fault (core dumped) zsh -fc ': ${${(PAA)p[foo]}::=x}'

Also happens when testing with machabot:

> 19:42 <jp> > : ${${(PAA)p[foo]}::=x}
> 19:42 <machabot> jp: zsh[248]: segfault at 0 ip b7dfcda3 sp bfeb9ebc
>       error 4 in libc-2.13.so[b7d84000+149000]

Add a simple `dupstring(s)` fallback.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
Requested-by: Bart Schaefer <schaefer@brasslantern.com>

 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/subst.c b/Src/subst.c
index d027e3d83cadc631a7..a265a187e8730868a9 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2430,7 +2430,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 		val = aval[0];
 		isarr = 0;
 	    }
-	    s = dyncat(val, s);
+	    s = val ? dyncat(val, s) : dupstring(s);
 	    /* Now behave po-faced as if it was always like that... */
 	    subexp = 0;
 	    /*
-- 
2.16.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] jp: fix segfaults during parameter expansion

[Bart Schaefer]

On Jan 21, 10:28am, Joey Pabalinas wrote:
}
} When i send updated patches, what's the best format? A new email after
} a separate response mail, or as a single email with the response
} at the bottom (or some other third option, maybe)?

Typically replying below/inline with an edited excerpt of the message
is preferred (as I believe you have been doing) rather than top-posting.
Also generally one explains the patch first and puts the patch at the
bottom, rather than having the patch in the middle of the text, although
there can be common-sense exceptions if there's something about the
code that's easier to explain after it has been seen.

It's not necessary to go back to the start of the thread to respond
separately/directly to that.  Better if the patch comes at the end of
the discussion context that brought it about.

Re: [PATCH] jp: fix segfaults during parameter expansion

[Joey Pabalinas]

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

On Sun, Jan 21, 2018 at 02:42:56PM -0800, Bart Schaefer wrote:
>
> Also generally one explains the patch first and puts the patch at the
> bottom, rather than having the patch in the middle of the text, although
> there can be common-sense exceptions if there's something about the
> code that's easier to explain after it has been seen.

Ah ok, gotcha. Will do it like that from now on.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]