PATCH: spelling correction buffer overflow

PATCH: spelling correction buffer overflow

[Oliver Kiddle]

In utils.c:spname(), there's no checking on newname[] for overflows in
two out the of three places where it is appended to.

returning NULL appears to abort the command-line rather than aborting
only correction which is perhaps not ideal but the aim here is not to
corrupt other bits of memory.

This also tweaks the struncpy() function I updated in the earlier patch.

Oliver

diff --git a/Src/utils.c b/Src/utils.c
index 998b16220..9ea34ab54 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2283,7 +2283,8 @@ struncpy(char **s, char *t, int n)
 {
     char *u = *s;
 
-    while (n-- && (*u++ = *t++));
+    while (n-- && (*u = *t++))
+	u++;
     *s = u;
     if (n > 0) /* just one null-byte will do, unlike strncpy(3) */
 	*u = '\0';
@@ -4420,17 +4421,20 @@ spname(char *oldname)
 	     * odd to the human reader, and we may make use of the total  *
 	     * distance for all corrections at some point in the future.  */
 	    if (bestdist < maxthresh) {
-		strcpy(new, spnameguess);
-		strcat(new, old);
-		return newname;
+		struncpy(&new, spnameguess, sizeof(newname) - (new - newname));
+		struncpy(&new, old, sizeof(newname) - (new - newname));
+		return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
 	    } else
 	    	return NULL;
 	} else {
 	    maxthresh = bestdist + thresh;
 	    bestdist += thisdist;
 	}
-	for (p = spnamebest; (*new = *p++);)
+	for (p = spnamebest; (*new = *p++);) {
+	    if ((new - newname) >= (sizeof(newname)-1))
+		return NULL;
 	    new++;
+	}
     }
 }
 

Re: PATCH: spelling correction buffer overflow

[Kamil Dudka]

On Tuesday, March 27, 2018 12:39:55 AM CEST Oliver Kiddle wrote:
> In utils.c:spname(), there's no checking on newname[] for overflows in
> two out the of three places where it is appended to.

Thanks!  After applying this patch, zsh does not crash any more while
trying to correct an overly long command.

Kamil

> returning NULL appears to abort the command-line rather than aborting
> only correction which is perhaps not ideal but the aim here is not to
> corrupt other bits of memory.
> 
> This also tweaks the struncpy() function I updated in the earlier patch.
> 
> Oliver
> 
> diff --git a/Src/utils.c b/Src/utils.c
> index 998b16220..9ea34ab54 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -2283,7 +2283,8 @@ struncpy(char **s, char *t, int n)
>  {
>      char *u = *s;
> 
> -    while (n-- && (*u++ = *t++));
> +    while (n-- && (*u = *t++))
> +	u++;
>      *s = u;
>      if (n > 0) /* just one null-byte will do, unlike strncpy(3) */
>  	*u = '\0';
> @@ -4420,17 +4421,20 @@ spname(char *oldname)
>  	     * odd to the human reader, and we may make use of the total  *
>  	     * distance for all corrections at some point in the future.  */
>  	    if (bestdist < maxthresh) {
> -		strcpy(new, spnameguess);
> -		strcat(new, old);
> -		return newname;
> +		struncpy(&new, spnameguess, sizeof(newname) - (new - newname));
> +		struncpy(&new, old, sizeof(newname) - (new - newname));
> +		return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
>  	    } else
>  	    	return NULL;
>  	} else {
>  	    maxthresh = bestdist + thresh;
>  	    bestdist += thisdist;
>  	}
> -	for (p = spnamebest; (*new = *p++);)
> +	for (p = spnamebest; (*new = *p++);) {
> +	    if ((new - newname) >= (sizeof(newname)-1))
> +		return NULL;
>  	    new++;
> +	}
>      }
>  }

Re: PATCH: spelling correction buffer overflow

[Bart Schaefer]

On Tue, Mar 27, 2018 at 2:55 PM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Tuesday, March 27, 2018 12:39:55 AM CEST Oliver Kiddle wrote:
>> In utils.c:spname(), there's no checking on newname[] for overflows in
>> two out the of three places where it is appended to.
>
> Thanks!  After applying this patch, zsh does not crash any more while
> trying to correct an overly long command.

I have not seen this patch on the -workers list, yet.

Re: PATCH: spelling correction buffer overflow

[Kamil Dudka]

On Wednesday, March 28, 2018 1:10:41 AM CEST Bart Schaefer wrote:
> On Tue, Mar 27, 2018 at 2:55 PM, Kamil Dudka <kdudka@redhat.com> wrote:
> > On Tuesday, March 27, 2018 12:39:55 AM CEST Oliver Kiddle wrote:
> >> In utils.c:spname(), there's no checking on newname[] for overflows in
> >> two out the of three places where it is appended to.
> > 
> > Thanks!  After applying this patch, zsh does not crash any more while
> > trying to correct an overly long command.
> 
> I have not seen this patch on the -workers list, yet.

I meant this patch (workers/42539):

http://www.zsh.org/mla/workers/2018/msg00345.html

Kamil

Re: PATCH: spelling correction buffer overflow

[Kamil Dudka]

On Tuesday, March 27, 2018 11:55:50 PM CEST Kamil Dudka wrote:
> On Tuesday, March 27, 2018 12:39:55 AM CEST Oliver Kiddle wrote:
> > In utils.c:spname(), there's no checking on newname[] for overflows in
> > two out the of three places where it is appended to.
> 
> Thanks!  After applying this patch, zsh does not crash any more while
> trying to correct an overly long command.

I spotted that this patch introduced new compiler warnings:

Src/utils.c:4430:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
#   return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
#                          ^

Src/utils.c:4438:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
#      if ((new - newname) >= (sizeof(newname)-1))
#                          ^

The full report is available at:
https://travis-ci.org/kdudka/zsh/builds/359289437#L2837

Should we cast RHS of the >= operator to ssize_t or ptrdiff_t to avoid them?
 
> Kamil
> 
> > returning NULL appears to abort the command-line rather than aborting
> > only correction which is perhaps not ideal but the aim here is not to
> > corrupt other bits of memory.
> > 
> > This also tweaks the struncpy() function I updated in the earlier patch.
> > 
> > Oliver
> > 
> > diff --git a/Src/utils.c b/Src/utils.c
> > index 998b16220..9ea34ab54 100644
> > --- a/Src/utils.c
> > +++ b/Src/utils.c
> > @@ -2283,7 +2283,8 @@ struncpy(char **s, char *t, int n)
> > 
> >  {
> >  
> >      char *u = *s;
> > 
> > -    while (n-- && (*u++ = *t++));
> > +    while (n-- && (*u = *t++))
> > +	u++;
> > 
> >      *s = u;
> >      if (n > 0) /* just one null-byte will do, unlike strncpy(3) */
> >  	
> >  	*u = '\0';
> > 
> > @@ -4420,17 +4421,20 @@ spname(char *oldname)
> > 
> >  	     * odd to the human reader, and we may make use of the total  *
> >  	     * distance for all corrections at some point in the future.  */
> >  	    
> >  	    if (bestdist < maxthresh) {
> > 
> > -		strcpy(new, spnameguess);
> > -		strcat(new, old);
> > -		return newname;
> > +		struncpy(&new, spnameguess, sizeof(newname) - (new - newname));
> > +		struncpy(&new, old, sizeof(newname) - (new - newname));
> > +		return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
> > 
> >  	    } else
> >  	    
> >  	    	return NULL;
> >  	
> >  	} else {
> >  	
> >  	    maxthresh = bestdist + thresh;
> >  	    bestdist += thisdist;
> >  	
> >  	}
> > 
> > -	for (p = spnamebest; (*new = *p++);)
> > +	for (p = spnamebest; (*new = *p++);) {
> > +	    if ((new - newname) >= (sizeof(newname)-1))
> > +		return NULL;
> > 
> >  	    new++;
> > 
> > +	}
> > 
> >      }
> >  
> >  }

Re: PATCH: spelling correction buffer overflow

[Oliver Kiddle]

Kamil Dudka wrote:
> I spotted that this patch introduced new compiler warnings:
>
> Src/utils.c:4430:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
> #   return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;

-Wsign-compare is not on by default and turning it on results in quite a
few warnings for the zsh code. It seems a little silly given that the
compiler ought to be able evaluate sizeof() at compile time and
establish that it is within the range of a signed integer.

> Should we cast RHS of the >= operator to ssize_t or ptrdiff_t to avoid them?

Casts are a bit ugly. The following - adding newname to both the LHS
and RHS - appears to avoid the warning by making both sides of the
comparison of type signed. At least until someone decides that adding an
unsigned to the signed should also generate a warning.

Oliver

diff --git a/Src/utils.c b/Src/utils.c
index eab407eee..3587c3622 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -4396,7 +4396,7 @@ spname(char *oldname)
      * Rationale for this, if there ever was any, has been forgotten.    */
     for (;;) {
 	while (*old == '/') {
-	    if ((new - newname) >= (sizeof(newname)-1))
+            if (new >= newname + sizeof(newname) - 1)
 		return NULL;
 	    *new++ = *old++;
 	}
@@ -4427,7 +4427,7 @@ spname(char *oldname)
 	    if (bestdist < maxthresh) {
 		struncpy(&new, spnameguess, sizeof(newname) - (new - newname));
 		struncpy(&new, old, sizeof(newname) - (new - newname));
-		return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
+		return (new >= newname + sizeof(newname) - 1) ? NULL : newname;
 	    } else
 	    	return NULL;
 	} else {
@@ -4435,7 +4435,7 @@ spname(char *oldname)
 	    bestdist += thisdist;
 	}
 	for (p = spnamebest; (*new = *p++);) {
-	    if ((new - newname) >= (sizeof(newname)-1))
+	    if (new >= newname + sizeof(newname) - 1)
 		return NULL;
 	    new++;
 	}

Re: PATCH: spelling correction buffer overflow

[Kamil Dudka]

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

On Wednesday, March 28, 2018 4:07:24 PM CEST Oliver Kiddle wrote:
> Kamil Dudka wrote:
> -Wsign-compare is not on by default and turning it on results in quite a
> few warnings for the zsh code.

I know but it is not a reason to introduce new warnings.

> It seems a little silly given that the
> compiler ought to be able evaluate sizeof() at compile time and
> establish that it is within the range of a signed integer.

The compiler knows it.  However, the range of a signed integer is not the 
problem that the compiler warns you about.  The problem is that semantics
of the C language is not exactly intuitive here.

A real-world problem would occur if (new - newname) became negative (not
sure if the surrounding code allows it or not).  Then the condition would
be evaluated as if the negative number was higher than the positive number
on RHS.

Please have a look at the attached example.  It demonstrates the actual 
problem that the compiler warns you about.  Interestingly, this is not an 
undefined behavior.  It is mandated (for example) by the C99 specification.

> > Should we cast RHS of the >= operator to ssize_t or ptrdiff_t to avoid
> > them?
> Casts are a bit ugly. The following - adding newname to both the LHS
> and RHS - appears to avoid the warning by making both sides of the
> comparison of type signed.

Looks good to me.

Kamil

> At least until someone decides that adding an
> unsigned to the signed should also generate a warning.
> 
> Oliver
> 
> diff --git a/Src/utils.c b/Src/utils.c
> index eab407eee..3587c3622 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -4396,7 +4396,7 @@ spname(char *oldname)
>       * Rationale for this, if there ever was any, has been forgotten.    */
> for (;;) {
>  	while (*old == '/') {
> -	    if ((new - newname) >= (sizeof(newname)-1))
> +            if (new >= newname + sizeof(newname) - 1)
>  		return NULL;
>  	    *new++ = *old++;
>  	}
> @@ -4427,7 +4427,7 @@ spname(char *oldname)
>  	    if (bestdist < maxthresh) {
>  		struncpy(&new, spnameguess, sizeof(newname) - (new - newname));
>  		struncpy(&new, old, sizeof(newname) - (new - newname));
> -		return (new - newname) >= (sizeof(newname)-1) ? NULL : newname;
> +		return (new >= newname + sizeof(newname) - 1) ? NULL : newname;
>  	    } else
>  	    	return NULL;
>  	} else {
> @@ -4435,7 +4435,7 @@ spname(char *oldname)
>  	    bestdist += thisdist;
>  	}
>  	for (p = spnamebest; (*new = *p++);) {
> -	    if ((new - newname) >= (sizeof(newname)-1))
> +	    if (new >= newname + sizeof(newname) - 1)
>  		return NULL;
>  	    new++;
>  	}

[-- Attachment #2: sign-compare.c --]
[-- Type: text/x-csrc, Size: 360 bytes --]

#include <stddef.h>
#include <stdio.h>

int main()
{
    const char *p1 = "text";
    const char *p2 = p1 + 2;
    ptrdiff_t diff = p1 - p2;

    printf("diff = %td\n", diff);
    printf("sizeof(p2) - 1 = %zu\n", sizeof(p2) - 1);
    if (diff >= sizeof(p2) - 1)
        printf("diff >= sizeof(p2) - 1\n");
    else
        printf("diff < sizeof(p2) - 1\n");
}