Re: PATCH: spelling correction buffer overflow

[Kamil Dudka <kdudka@redhat.com>]

[-- 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");
}