Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20190806020320.GR9017@brightrain.aerifal.cx>
Date: Mon, 5 Aug 2019 22:03:20 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] In glob(), do not require that the target of a
 symlink exists.

On Mon, Jul 22, 2019 at 08:37:38PM -0400, James Y Knight wrote:
> Previously, when given a trailing path component with no
> metacharacters, glob would call stat to check if the provided filename
> existed, which would fail on a broken symlink. When expanding a
> pattern however, glob would trust the list of files returned by
> readdir, and thus would return broken symlinks.
> 
> Now, be consistent and allow broken symlinks in both cases, by using
> lstat to determine file existence.
> 
> If GLOB_MARK is specified, stat must still be used to determine
> whether a given name refers to a directory or file. But if that fails,
> a symlink is simply considered to be a file for marking purposes.

> From 7ef0fabf173f4d29461b01ccfb05c1f19977ba37 Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@...gle.com>
> Date: Thu, 11 Jul 2019 16:55:17 -0400
> Subject: [PATCH] In glob(), do not require that the target of a symlink
>  exists.
> 
> Previously, when given a trailing path component with no
> metacharacters, glob would call stat to check if the provided filename
> existed, which would fail on a broken symlink. When expanding a
> pattern however, glob would trust the list of files returned by
> readdir, and thus would return broken symlinks.
> 
> Now, be consistent and allow broken symlinks in both cases, by using
> lstat to determine file existence.
> 
> If GLOB_MARK is specified, stat must still be used to determine
> whether a given name refers to a directory or file. But if that fails,
> a symlink is simply considered to be a file for marking purposes.
> ---
>  src/regex/glob.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/src/regex/glob.c b/src/regex/glob.c
> index aa1c6a44..01b178dc 100644
> --- a/src/regex/glob.c
> +++ b/src/regex/glob.c
> @@ -88,18 +88,33 @@ static int do_glob(char *buf, size_t pos, int type, char *pat, int flags, int (*
>  	}
>  	buf[pos] = 0;
>  	if (!*pat) {
> -		/* If we consumed any components above, or if GLOB_MARK is
> -		 * requested and we don't yet know if the match is a dir,
> -		 * we must call stat to confirm the file exists and/or
> -		 * determine its type. */
> +		/* If we're in at the end of the pattern, check if the file
> +		 * exists. And, if GLOB_MARK is requested, determine if it's a
> +		 * directory. */

This is one of the comment changes I was confused about; it seems less
informative. The point of the original comment was not describing the
controlling expression just above it, but the conditions on not being
able to use the caller-passed type -- having consumed any new literal
components from the pattern above is what precludes it.

>  		struct stat st;
> -		if ((flags & GLOB_MARK) && type==DT_LNK) type = 0;
> -		if (!type && stat(buf, &st)) {
> -			if (errno!=ENOENT && (errfunc(buf, errno) || (flags & GLOB_ERR)))
> -				return GLOB_ABORTED;
> -			return 0;
> +		if (flags & GLOB_MARK) {
> +			/* We need to know if the name refers to a directory
> +			 * (after resolving symlinks).
> +			 *
> +			 * However, broken symlinks should be considered to be a
> +			 * file, rather than non-existent or an error, so if
> +			 * stat fails, we just don't modify type. (And lstat
> +			 * will be called below if required.)
> +			 */
> +			if ((!type || type==DT_LNK) && stat(buf, &st) == 0) {

!stat

> +				if(S_ISDIR(st.st_mode)) type = DT_DIR;
> +				else type = DT_REG;
> +			}
> +		}
> +		if (!type) {
> +			/* If we're don't already know that the file exists,
> +			 * confirm its presence. */
> +			if (lstat(buf, &st)) {

This could be "!type && lstat(..." to avoid the excessively long lines
below and to better mimic the old structure so that stylistic change
doesn't obscure reading of functional change.

Now that I look at it, the same applies to the above too.

> +				if (errno!=ENOENT && (errfunc(buf, errno) || (flags & GLOB_ERR)))
> +					return GLOB_ABORTED;
> +				return 0;
> +			}
>  		}
> -		if (!type && S_ISDIR(st.st_mode)) type = DT_DIR;
>  		if (append(tail, buf, pos, (flags & GLOB_MARK) && type==DT_DIR))
>  			return GLOB_NOSPACE;
>  		return 0;
> -- 
> 2.22.0.657.g960e92d24f-goog
> 

Otherwise I think this looks ok.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.