Re: zsh 5.0.5-dev-2

[Han Pingtian <hanpt@xxxxxxxxxxxxxxxxxx>]

Hi,

Looks like on the 747 line of Src/utils.c:

747         sprintf(xbuf2, "%s/%s", xbuf, *pp);

The "cd .." will trigger a buffer overflow if I compile zsh with 
-D FORTIFY_SOURCE=2 . Shall we return -1 here if it will overflow xbuf2?

On Thu, Aug 14, 2014 at 09:44:12PM -0700, Bart Schaefer wrote:
> On Aug 14,  8:54pm, Peter Stephenson wrote:
> }
> } It's entirely unclear to me from looking at xsymlinks() when an empty
> } xbuf would actually constitute a failure.
> 
> This occurs in the (t0 == -1) case when the reason that readlink() has
> failed is because the path so far concatenated with the next segment of
> the split path has produced a string longer than PATH_MAX.  In that case
> (xbuflen + pplen > sizeof(xbuf)) we completely truncate xbuf and give up.
> 
> } If it can't expand something
> } as a symbolic link it simply treats it literally;
> 
> Yes, but for a /path/to/somereallylongname, appending somereallylongname
> to the expansion of xbuf so far, might overflow xbuf.
> 
> } If we don't have an answer now I'd be inclined to comment out the
> } warning.
> 
> Maybe just make xsymlinks() have a ternary return value instead of a
> boolean one, and return an actual error value in the case where we've
> truncated on overflow instead of as part of normal expansion to root?
> 
> I don't know if all of these changes are necessary.  It has always
> bothered me that the return value of the recursive calls to xsymlinks()
> are ignored, but maybe this isn't the right way to handle them.
> 
> Incidentally, anybody recall what the obviously obsolete comment above
> the xsymlinks() definition in Src/utils.c is about?
> 
> diff --git a/Src/utils.c b/Src/utils.c
> index 998e46a..076a33c 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -716,7 +716,6 @@ slashsplit(char *s)
>  }
> 
>  /* expands symlinks and .. or . expressions */
> -/* if flag = 0, only expand .. and . expressions */
> 
>  /**/
>  static int
> @@ -753,6 +752,7 @@ xsymlinks(char *s)
>  		strcat(xbuf, *pp);
>  	    } else {
>  		*xbuf = 0;
> +		ret = -1;
>  		break;
>  	    }
>  	} else {
> @@ -760,9 +760,11 @@ xsymlinks(char *s)
>  	    metafy(xbuf3, t0, META_NOALLOC);
>  	    if (*xbuf3 == '/') {
>  		strcpy(xbuf, "");
> -		xsymlinks(xbuf3 + 1);
> +		if (xsymlinks(xbuf3 + 1) < 0)
> +		    ret = -1;
>  	    } else
> -		xsymlinks(xbuf3);
> +		if (xsymlinks(xbuf3) < 0)
> +		    ret = -1;
>  	}
>      }
>      freearray(opp);
> @@ -781,11 +783,10 @@ xsymlink(char *s)
>      if (*s != '/')
>  	return NULL;
>      *xbuf = '\0';
> -    xsymlinks(s + 1);
> -    if (!*xbuf) {
> +    if (xsymlinks(s + 1) < 0)
>  	zwarn("path expansion failed, using root directory");
> +    if (!*xbuf)
>  	return ztrdup("/");
> -    }
>      return ztrdup(xbuf);
>  }
> 
> @@ -795,7 +796,7 @@ print_if_link(char *s)
>  {
>      if (*s == '/') {
>  	*xbuf = '\0';
> -	if (xsymlinks(s + 1))
> +	if (xsymlinks(s + 1) > 0)
>  	    printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout);
>      }
>  }
> 
> -- 
> Barton E. Schaefer