From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8705 invoked by alias); 19 Apr 2014 20:41:41 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 8695 invoked by uid 89); 19 Apr 2014 20:41:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx3-phx2.redhat.com Received: from mx3-phx2.redhat.com (HELO mx3-phx2.redhat.com) (209.132.183.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 19 Apr 2014 20:41:40 +0000 Received: from zmail14.collab.prod.int.phx2.redhat.com (zmail14.collab.prod.int.phx2.redhat.com [10.5.83.16]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s3JKfckF028468; Sat, 19 Apr 2014 16:41:38 -0400 Date: Sun, 20 Apr 2014 06:07:00 -0000 From: Kai Tietz To: Ray Donnelly Cc: gcc-patches@gcc.gnu.org, ktietz70@gmail.com Message-ID: <1034557352.9542218.1397940098399.JavaMail.zimbra@redhat.com> In-Reply-To: <1397936424-2290-3-git-send-email-mingw.android@gmail.com> References: <1397936424-2290-1-git-send-email-mingw.android@gmail.com> <1397936424-2290-3-git-send-email-mingw.android@gmail.com> Subject: Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg01134.txt.bz2 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 > + > + * files.c (open_file): Implement Posix existence > + semantics for paths containing '../' > + > 2014-02-24 Walter Lee > > * 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 > > +/* Needed for stat_st_mode_symlink below */ > +#if defined(_WIN32) > +# include > +# 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 , 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