[PATCH 0/3] jp: Patchset for parameter expansion segfaults

[PATCH 0/3] jp: Patchset for parameter expansion segfaults

[Joey Pabalinas]

Add checks to guard against NULL-deref-caused segfaults in current
zsh parameter-expansion edge-cases.

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

Joey Pabalinas (3):
  - Fix segfaults during parameter expansion
  - Use `(nil)` for empty identifier strings
  - Add `dupstring()` fallback to `zhtricat()`

 Src/params.c |  2 ++
 Src/string.c | 23 +++++++++++++++++++----
 Src/subst.c  |  4 ++++
 3 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH 1/3] jp: Fix segfaults during parameter expansion

[Joey Pabalinas]

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(s2)` fallback instead of pointlessly
trying to concatenate `s2` to NULL and segfaulting.

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

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

diff --git a/Src/string.c b/Src/string.c
index 9e14ef94919c3e8ec5..038624d65a9f533494 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -144,8 +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;
 
+    /* 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 +162,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);
-- 
2.15.1

[PATCH 2/3] jp: Use `(nil)` for empty identifier strings

[Joey Pabalinas]

Indicate the presence of an empty identifier string using `(nil)`; the
empty string case should still provide a somewhat useful error message using:

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

rather than:

> zsh:1: not an identifier:

which is fairly confusing and sort of useless.

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

 2 files changed, 6 insertions(+)

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/subst.c b/Src/subst.c
index d027e3d83cadc631a7..5fe69a6b94bfad8d9e 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

Re: [PATCH 2/3] jp: Use `(nil)` for empty identifier strings

[Daniel Shahaf]

Joey Pabalinas wrote on Sun, 14 Jan 2018 05:23 -1000:
> +++ 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);

How about inventing an %ls expando that emits the argument quoted, so
zerr("%ls", foo) would print ${(qq)foo} or ${(q-)foo}.  That seems easier
than updating all callsites (not just these three).

(%ls/%s by analogy with %ld/%d.)

Cheers,

Daniel

Re: [PATCH 2/3] jp: Use `(nil)` for empty identifier strings

[Joey Pabalinas]

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

On Sun, Jan 14, 2018 at 03:43:08PM +0000, Daniel Shahaf wrote:
>
> How about inventing an %ls expando that emits the argument quoted, so
> zerr("%ls", foo) would print ${(qq)foo} or ${(q-)foo}.  That seems easier
> than updating all callsites (not just these three).
> 
> (%ls/%s by analogy with %ld/%d.)

If only it were so easy :( All that `zerr()` does is pass the
`va_list` to `zerrmsg()` and i didn't want to touch too many
things.

I like the idea though, actually. So if i messed around with
implementing things in `zerrmsg()` would the "" or '' format
be better than bothering with (nil) at all?

-- 
Joey Pabalinas

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

Re: [PATCH 2/3] jp: Use `(nil)` for empty identifier strings

[Bart Schaefer]

On Sun, Jan 14, 2018 at 7:23 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> Indicate the presence of an empty identifier string using `(nil)`; the
> empty string case should still provide a somewhat useful error message using:
>
>> zsh:1: not an identifier: (nil)
>
> rather than:
>
>> zsh:1: not an identifier:
>
> which is fairly confusing and sort of useless.

We use "(nil)" literally nowhere else.  This needs some discussion,
especially before any effort is put into changing any of the
zerr*/zwarn* functions.

[PATCH 3/3] jp: Add `dupstring()` fallback to `zhtricat()`

[Joey Pabalinas]

Add `dupstring()` fallback machanism to guard against NULL derefs
in 3-argument concat function.

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

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

diff --git a/Src/string.c b/Src/string.c
index 038624d65a9f533494..df7e932061dbfbaab2 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -126,9 +126,16 @@ 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, 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);
-- 
2.15.1

Re: [PATCH 3/3] jp: Add `dupstring()` fallback to `zhtricat()`

[Joey Pabalinas]

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

On Sun, Jan 14, 2018 at 05:23:44AM -1000, Joey Pabalinas wrote:
> Add `dupstring()` fallback machanism to guard against NULL derefs
> in 3-argument concat function.

I am not 100% sure about this patch, so I split it off along with
the error message changes; feel free to drop individual patches or
suggest improvements.

On Sun, Jan 14, 2018 at 05:23:44AM -1000, Joey Pabalinas wrote:
> diff --git a/Src/string.c b/Src/string.c
> index 038624d65a9f533494..df7e932061dbfbaab2 100644
> --- a/Src/string.c
> +++ b/Src/string.c
> @@ -126,9 +126,16 @@ 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, 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);
> -- 
> 2.15.1

-- 
Joey Pabalinas

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

Re: [PATCH 3/3] jp: Add `dupstring()` fallback to `zhtricat()`

[Bart Schaefer]

On Jan 14,  5:29am, Joey Pabalinas wrote:
}
} On Sun, Jan 14, 2018 at 05:23:44AM -1000, Joey Pabalinas wrote:
} > Add `dupstring()` fallback machanism to guard against NULL derefs
} > in 3-argument concat function.
} 
} I am not 100% sure about this patch, so I split it off along with
} the error message changes; feel free to drop individual patches or
} suggest improvements.

I suspect we're not going to include this patch anyway, but in case
anyone is trying it out, please not it only catches null pointer in
the first two of the three arguments to zshtricat().

Re: [PATCH 0/3] jp: Patchset for parameter expansion segfaults

[Bart Schaefer]

On Sun, Jan 14, 2018 at 7:23 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
>
> Joey Pabalinas (3):
>   - Fix segfaults during parameter expansion
>   - Use `(nil)` for empty identifier strings
>   - Add `dupstring()` fallback to `zhtricat()`

With appreciation for your efforts here, preventing segfaults is not
the correct goal.  The goal should be that the software produces the
correct results.

Trapping bad pointers in the string copy/catenate routines potentially
masks more serious errors; it introduces what only appears to be
error-free operation in cases that probably ought to fail.  A segfault
in zhtricat() is nearly always an indication that the calling code is
doing something wrong, and covering that up only makes it harder to
find what that is.

At the very least any such "succeed in spite of caller screwup" code
should be wrapped in #ifndef DEBUG or the like.

Re: [PATCH 0/3] jp: Patchset for parameter expansion segfaults

[Joey Pabalinas]

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

On Sun, Jan 14, 2018 at 12:41:05PM -0800, Bart Schaefer wrote:
> With appreciation for your efforts here, preventing segfaults is not
> the correct goal.  The goal should be that the software produces the
> correct results.
> 
> Trapping bad pointers in the string copy/catenate routines potentially
> masks more serious errors; it introduces what only appears to be
> error-free operation in cases that probably ought to fail.  A segfault
> in zhtricat() is nearly always an indication that the calling code is
> doing something wrong, and covering that up only makes it harder to
> find what that is.
> 
> At the very least any such "succeed in spite of caller screwup" code
> should be wrapped in #ifndef DEBUG or the like.

No worries, I understand and wholeheartedly agree with that
philosophy. It would definitely be better to go over the
calling code and figure out exactly where the wrong assumptions
are being made with respect to intended behavior.

When I have a bit more free time I'll hopefully do just that, cheers.

-- 
Joey Pabalinas

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