Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Kai Tietz <ktietz at redhat dot com>]

Hello Ray,

----- Original Message -----
> Windows does a short-circuit lookup of paths containing
> ../ which means that:
> 
> exists/doesnotexist/../file
> 
> is considered to exist, while on Posix it is considered
> not to. The Posix semantics are relied upon when building
> glibc so any paths containing "../" are checked component
> wise.
> 
> libcpp/
> 	* files.c (open_file): Implement Posix existence
> 	semantics for paths containing '../'
> ---
>  libcpp/ChangeLog |  5 ++++
>  libcpp/files.c   | 86
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
> index 3a63434..ae1b62a 100644
> --- a/libcpp/ChangeLog
> +++ b/libcpp/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-14  Ray Donnelly  <mingw.android@gmail.com>
> +
> +	* files.c (open_file): Implement Posix existence
> +	semantics for paths containing '../'
> +
>  2014-02-24  Walter Lee  <walt@tilera.com>
>  
>  	* configure.ac: Change "tilepro" triplet to "tilepro*".

As for the other patch, please don't include ChangeLog modifications to the diff.  Instead just write them in mail.

> diff --git a/libcpp/files.c b/libcpp/files.c
> index 7e88778..a9326bf 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -30,6 +30,13 @@ along with this program; see the file COPYING3.  If not
> see
>  #include "md5.h"
>  #include <dirent.h>
>  
> +/* Needed for stat_st_mode_symlink below */
> +#if defined(_WIN32)
> +#  include <windows.h>
> +#  define S_IFLNK 0xF000
> +#  define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
> +#endif
> +
>  /* Variable length record files on VMS will have a stat size that includes
>     record control characters that won't be included in the read size.  */
>  #ifdef VMS
> @@ -198,6 +205,49 @@ static int pchf_save_compare (const void *e1, const void
> *e2);
>  static int pchf_compare (const void *d_p, const void *e_p);
>  static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
>  
> +#if defined(_WIN32)

This check isn't enough here.  you should check additionally that it isn't cygwin-host (__CYWIN__).

> +
> +static int stat_st_mode_symlink (char const* path, struct stat* buf)
Please break line after 'int' for coding-style.
> +{
> +  WIN32_FILE_ATTRIBUTE_DATA attr;
> +  memset(buf, 0, sizeof(*buf));
> +  int err = GetFileAttributesExA (path, GetFileExInfoStandard, &attr) ? 0 :
> 1;
> +  if (!err)
> +    {
> +      WIN32_FIND_DATAA finddata;
> +      HANDLE h = FindFirstFileA (path, &finddata);
Add an new line here.
> +      if (h != INVALID_HANDLE_VALUE)
> +        {
> +          FindClose (h);
> +          if ((finddata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> +              (finddata.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
> +              buf->st_mode = S_IFLNK;
> +          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> +              buf->st_mode = S_IFDIR;
> +          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_ARCHIVE)
> +              buf->st_mode = S_IFDIR;
> +          else
> +              buf->st_mode = S_IFREG;
> +          buf->st_mode |= S_IREAD;
> +          if (!(finddata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> +              buf->st_mode |= S_IWRITE;
> +        }
> +      else
> +        {
> +          buf->st_mode = S_IFDIR;
> +        }
> +      return 0;
> +    }
> +  return -1;
> +}
> +
> +#else
> +
> +#define stat_st_mode_symlink (_name, _buf) stat ((_name), (_buf))
> +
> +#endif

Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
> +
> +
>  /* Given a filename in FILE->PATH, with the empty string interpreted
>     as <stdin>, open it.
>  
> @@ -227,6 +277,42 @@ open_file (_cpp_file *file)
>      }
>    else
>      file->fd = open (file->path, O_RDONLY | O_NOCTTY | O_BINARY, 0666);
> +#if defined(_WIN32) || defined(__CYGWIN__)
> +  /* Windows and Posix differ in the face of paths of the form:
> +     nonexistantdir/.. in that Posix will return ENOENT whereas
> +     Windows won't care that we stepped into a non-existant dir
> +     Only do these slow checks if "../" appears in file->path.
> +     Cygwin also suffers from the same problem (but doesn't need
> +     a new stat function):
> +     http://cygwin.com/ml/cygwin/2013-05/msg00222.html
> +  */
> +  if (file->fd > 0)
> +    {
> +      char filepath[MAX_PATH];
> +      strncpy (filepath, file->path, MAX_PATH);
> +      filepath[MAX_PATH-1] = (char) 0;
> +      char *dirsep = &filepath[0];
Please add here a new-line.
> +      while ( (dirsep = strchr (dirsep, '\\')) != NULL)
Space after '(' needs to be removed.
> +        *dirsep = '/';
> +      if (strstr(filepath, "../"))
> +	{
> +	  dirsep = &filepath[0];
> +	  /* Check each directory in the chain. */
Comments should end with two spaces.
> +	  while ( (dirsep = strchr (dirsep, '/')) != NULL)
Same as above.  No space after '(' here.
> +	    {
> +	      *dirsep = (char) 0;
this case here looks to me bogus, ... if you want to indicate it is a character then you might want to use here '\0' instead.
> +	      if (stat_st_mode_symlink (filepath, &file->st) == -1)
> +	        {
> +	          *dirsep = '/';
> +	          close (file->fd);
> +	          file->fd = -1;
> +	          return false;
> +	        }
Here should come a newline.
> +	      *dirsep = '/';
> +	    }
> +	}
> +    }
> +#endif
>  
>    if (file->fd != -1)
>      {
> --
> 1.9.2
> 
> 

The implemented logic of this patch makes sense and is IMO ok.  There are some minor coding-style issues you need to correct, and  I would recomment to split this patch into 2 parts (libiberty and libcpp).

Cheers,
Kai