Re: PATCH: spelling correction buffer overflow

[Kamil Dudka <kdudka@xxxxxxxxxx>]

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++;
> > 
> > +	}
> > 
> >      }
> >  
> >  }