Re: PATCH: spelling correction buffer overflow

[Kamil Dudka <kdudka@redhat.com>]

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