[patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 171 bytes --]

Solaris 10 doesn't follow POSIX in accepting a null pointer as the
second argument to realpath(), so allocate a buffer for it.

Tested x86_64-linux, committed to trunk.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1214 bytes --]

commit ed4023452f85f6c745ce473b2503f4e46fb02cd9
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Sep 11 15:19:27 2015 +0100

    Fix filesystem::canonical on Solaris 10.
    
    	PR libstdc++/67173
    	* src/filesystem/ops.cc (filesystem::canonical): Allocate buffer for
    	realpath on Solaris 10.

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 661685a..cefb927 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -28,6 +28,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <errno.h>
+#include <limits.h>  // PATH_MAX
 #ifdef _GLIBCXX_HAVE_UNISTD_H
 # include <unistd.h>
 # if defined(_GLIBCXX_HAVE_SYS_STAT_H) && defined(_GLIBCXX_HAVE_SYS_TYPES_H)
@@ -97,7 +98,11 @@ fs::canonical(const path& p, const path& base, error_code& ec)
 {
   path can;
 #ifdef _GLIBCXX_USE_REALPATH
-  if (char_ptr rp = char_ptr{::realpath(absolute(p, base).c_str(), nullptr)})
+  char* buffer = nullptr;
+#if defined(__SunOS_5_10) && defined(PATH_MAX)
+  buffer = (char*)::malloc(PATH_MAX);
+#endif
+  if (char_ptr rp = char_ptr{::realpath(absolute(p, base).c_str(), buffer)})
     {
       can.assign(rp.get());
       ec.clear();

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

On 09/11/2015 08:21 AM, Jonathan Wakely wrote:
> Solaris 10 doesn't follow POSIX in accepting a null pointer as the
> second argument to realpath(), so allocate a buffer for it.

FWIW, the NULL requirement is new in Issue 7. In Issue 6, the behavior
is implementation-defined, and before then it was an error. Solaris 10
claims conformance to SUSv2 and its realpath fails with EINVAL.
Solaris 11 says it conforms to Issue 6 but according to the man page
its realpath already implements the Issue 7 requirement.

I suspect the same problem will come up on other systems so checking
the POSIX version might be better than testing for each OS.

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 11 September 2015 at 18:39, Martin Sebor wrote:
> On 09/11/2015 08:21 AM, Jonathan Wakely wrote:
>>
>> Solaris 10 doesn't follow POSIX in accepting a null pointer as the
>> second argument to realpath(), so allocate a buffer for it.
>
>
> FWIW, the NULL requirement is new in Issue 7. In Issue 6, the behavior
> is implementation-defined, and before then it was an error. Solaris 10
> claims conformance to SUSv2 and its realpath fails with EINVAL.
> Solaris 11 says it conforms to Issue 6 but according to the man page
> its realpath already implements the Issue 7 requirement.

Thanks.

> I suspect the same problem will come up on other systems so checking
> the POSIX version might be better than testing for each OS.

The problem with doing that is that many BSD systems have supported
passing NULL as an extension long before issue 7, so if we just check
something like _POSIX_VERSION >= 200809L then we can only canonicalize
paths up to PATH_MAX on many systems where the extension is available
but _POSIX_VERSION implies conformance to an older standard.

So maybe we want an autoconf macro saying whether realpath() accepts
NULL, and just hardcode it for the targets known to support it, and
only check _POSIX_VERSION for the unknowns.

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

On 09/12/2015 04:09 AM, Jonathan Wakely wrote:
> On 11 September 2015 at 18:39, Martin Sebor wrote:
>> On 09/11/2015 08:21 AM, Jonathan Wakely wrote:
>>>
>>> Solaris 10 doesn't follow POSIX in accepting a null pointer as the
>>> second argument to realpath(), so allocate a buffer for it.
>>
>>
>> FWIW, the NULL requirement is new in Issue 7. In Issue 6, the behavior
>> is implementation-defined, and before then it was an error. Solaris 10
>> claims conformance to SUSv2 and its realpath fails with EINVAL.
>> Solaris 11 says it conforms to Issue 6 but according to the man page
>> its realpath already implements the Issue 7 requirement.
>
> Thanks.
>
>> I suspect the same problem will come up on other systems so checking
>> the POSIX version might be better than testing for each OS.
>
> The problem with doing that is that many BSD systems have supported
> passing NULL as an extension long before issue 7, so if we just check
> something like _POSIX_VERSION >= 200809L then we can only canonicalize
> paths up to PATH_MAX on many systems where the extension is available
> but _POSIX_VERSION implies conformance to an older standard.

You're right. I agree that just checking the POSIX version may not
lead to optimal results. Some implementations also support multiple
versions and the one in effect may not be the one most recent. To
get the most out of those, it's usually recommended to set
_POSIX_C_SOURCE to the latest version before including any headers,
then test the supported version, and when the supported version is
less than the one requested and involves implementation defined
behavior (as in Issue 6) or undefined behavior that's known to be
used to provide extensions (as in SUSv2), check the implementation
version just as the patch does.

>
> So maybe we want an autoconf macro saying whether realpath() accepts
> NULL, and just hardcode it for the targets known to support it, and
> only check _POSIX_VERSION for the unknowns.

That will work for Issue 6 where the realpath behavior is
implementation-defined. The test wouldn't yield reliable results
for SUSv2 implementations where the behavior is undefined. There,
the result would have to be hardcoded based on what the manual says.
An autoconf test won't help with the ENAMETOOLONG problem since it
might depend on the filesystem. To overcome that, libstdc++ would
need to do the path traversal itself.

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

On 09/12/2015 12:07 PM, Martin Sebor wrote:
> On 09/12/2015 04:09 AM, Jonathan Wakely wrote:
>> On 11 September 2015 at 18:39, Martin Sebor wrote:
>>> On 09/11/2015 08:21 AM, Jonathan Wakely wrote:
>>>>
>>>> Solaris 10 doesn't follow POSIX in accepting a null pointer as the
>>>> second argument to realpath(), so allocate a buffer for it.
>>>
>>>
>>> FWIW, the NULL requirement is new in Issue 7. In Issue 6, the behavior
>>> is implementation-defined, and before then it was an error. Solaris 10
>>> claims conformance to SUSv2 and its realpath fails with EINVAL.
>>> Solaris 11 says it conforms to Issue 6 but according to the man page
>>> its realpath already implements the Issue 7 requirement.
>>
>> Thanks.
>>
>>> I suspect the same problem will come up on other systems so checking
>>> the POSIX version might be better than testing for each OS.
>>
>> The problem with doing that is that many BSD systems have supported
>> passing NULL as an extension long before issue 7, so if we just check
>> something like _POSIX_VERSION >= 200809L then we can only canonicalize
>> paths up to PATH_MAX on many systems where the extension is available
>> but _POSIX_VERSION implies conformance to an older standard.
>
> You're right. I agree that just checking the POSIX version may not
> lead to optimal results. Some implementations also support multiple
> versions and the one in effect may not be the one most recent. To
> get the most out of those, it's usually recommended to set
> _POSIX_C_SOURCE to the latest version before including any headers,
> then test the supported version, and when the supported version is
> less than the one requested and involves implementation defined
> behavior (as in Issue 6) or undefined behavior that's known to be
> used to provide extensions (as in SUSv2), check the implementation
> version just as the patch does.
>
>>
>> So maybe we want an autoconf macro saying whether realpath() accepts
>> NULL, and just hardcode it for the targets known to support it, and
>> only check _POSIX_VERSION for the unknowns.
>
> That will work for Issue 6 where the realpath behavior is
> implementation-defined. The test wouldn't yield reliable results
> for SUSv2 implementations where the behavior is undefined.

Sorry -- I meant "SUSv2 where the behavior is an error, or in
implementations where the behavior is undefined" (in general).

But based on what you said, the BSD implementations that accept
NULL are non-conforming so they would need to be treated as such
(i.e., outside of POSIX).

> There,
> the result would have to be hardcoded based on what the manual says.
> An autoconf test won't help with the ENAMETOOLONG problem since it
> might depend on the filesystem. To overcome that, libstdc++ would
> need to do the path traversal itself.
>
> Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 4242 bytes --]

On 12/09/15 12:07 -0600, Martin Sebor wrote:
>On 09/12/2015 04:09 AM, Jonathan Wakely wrote:
>>On 11 September 2015 at 18:39, Martin Sebor wrote:
>>>On 09/11/2015 08:21 AM, Jonathan Wakely wrote:
>>>>
>>>>Solaris 10 doesn't follow POSIX in accepting a null pointer as the
>>>>second argument to realpath(), so allocate a buffer for it.
>>>
>>>
>>>FWIW, the NULL requirement is new in Issue 7. In Issue 6, the behavior
>>>is implementation-defined, and before then it was an error. Solaris 10
>>>claims conformance to SUSv2 and its realpath fails with EINVAL.
>>>Solaris 11 says it conforms to Issue 6 but according to the man page
>>>its realpath already implements the Issue 7 requirement.
>>
>>Thanks.
>>
>>>I suspect the same problem will come up on other systems so checking
>>>the POSIX version might be better than testing for each OS.
>>
>>The problem with doing that is that many BSD systems have supported
>>passing NULL as an extension long before issue 7, so if we just check
>>something like _POSIX_VERSION >= 200809L then we can only canonicalize
>>paths up to PATH_MAX on many systems where the extension is available
>>but _POSIX_VERSION implies conformance to an older standard.
>
>You're right. I agree that just checking the POSIX version may not
>lead to optimal results. Some implementations also support multiple
>versions and the one in effect may not be the one most recent. To
>get the most out of those, it's usually recommended to set
>_POSIX_C_SOURCE to the latest version before including any headers,
>then test the supported version, and when the supported version is
>less than the one requested and involves implementation defined
>behavior (as in Issue 6) or undefined behavior that's known to be
>used to provide extensions (as in SUSv2), check the implementation
>version just as the patch does.
>
>>
>>So maybe we want an autoconf macro saying whether realpath() accepts
>>NULL, and just hardcode it for the targets known to support it, and
>>only check _POSIX_VERSION for the unknowns.
>
>That will work for Issue 6 where the realpath behavior is
>implementation-defined. The test wouldn't yield reliable results
>for SUSv2 implementations where the behavior is undefined. There,
>the result would have to be hardcoded based on what the manual says.
>An autoconf test won't help with the ENAMETOOLONG problem since it
>might depend on the filesystem. To overcome that, libstdc++ would
>need to do the path traversal itself.

It turns out I was wrong about BSD traditionally supporting
realpath(path, NULL), it first appeared in these relatively recent
versions:

FreeBSD 9.0
OpenBSD 5.0
NetBSD 6.1

Like Solaris 11, some of them still define _POSIX_VERSION=200112L even
though they support passing NULL, so we could hardcode the targets
that are known to support it, but we'll still need a fallback for lots
of slightly older targets anyway.
 
So here's a new implementation of canonical() which only uses
realpath() when this autoconf compile-or-link test passes:

#if _XOPEN_VERSION < 500
#error
#elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
char *tmp = realpath((const char*)NULL, (char*)NULL);
#else
#error
#endif

i.e. I use realpath() for Issue 7, or for Issue 6 if PATH_MAX is
defined.

Then in filesystem::canonical() if _GLIBCXX_USE_REALPATH is set I use
it, passing NULL for Issue 7, or allocating a buffer of PATH_MAX
otherwise. If realpath isn't supported or fails with ENAMETOOLONG then
I do the path traversal by hand (which can be done entirely using the
std::experimental::filesystem operations).

Only passing NULL for Issue 7 is quite conservative. It means we don't
do it for targets that support it as an implementation-defined
extension to Issue 6, which includes Solaris 11, the BSDs and even
older GNU systems (including RHEL6). But that's OK, we have a fallback
now so it means no loss of functionality, just efficiency.  We can
tweak the config later for targets known to handle NULL.

The new testcase is not very thorough. I've run a few more involved
tests that aren't suitable to check in until I figure out a good way
of running filesystem tests that can create/remove arbitrary files and
symlinks.

What do you think?

Tested powerpc64le-linux and x86_64-dragonfly4.2.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 7285 bytes --]

commit e79ad2dbcb14d1e66f6edead4ff87b62e575a8e7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 16 14:07:54 2015 +0100

    Implement filesystem::canonical() without realpath
    
    	PR libstdc++/67173
    	* acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check _XOPEN_VERSION
    	and PATH_MAX for _GLIBCXX_USE_REALPATH.
    	* config.h.in: Regenerate.
    	* configure: Regenerate.
    	* src/filesystem/dir.cc: Define _XOPEN_VERSION.
    	* src/filesystem/ops.cc: Likewise.
    	(canonical) [!_GLIBCXX_USE_REALPATH]: Add alternative implementation.
    	* testsuite/experimental/filesystem/operations/canonical.cc: New.

diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 64c9b7e..364a7d2 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -3926,7 +3926,7 @@ dnl
   AC_LANG_SAVE
   AC_LANG_CPLUSPLUS
   ac_save_CXXFLAGS="$CXXFLAGS"
-  CXXFLAGS="$CXXFLAGS -fno-exceptions"
+  CXXFLAGS="$CXXFLAGS -fno-exceptions -D_XOPEN_SOURCE=700"
 dnl
   AC_MSG_CHECKING([for struct dirent.d_type])
   AC_CACHE_VAL(glibcxx_cv_dirent_d_type, [dnl
@@ -3947,13 +3947,24 @@ dnl
   AC_MSG_CHECKING([for realpath])
   AC_CACHE_VAL(glibcxx_cv_realpath, [dnl
     GCC_TRY_COMPILE_OR_LINK(
-      [#include <stdlib.h>],
-      [char *tmp = realpath((const char*)NULL, (char*)NULL);],
+      [
+       #include <stdlib.h>
+       #include <unistd.h>
+      ],
+      [
+       #if _XOPEN_VERSION < 500
+       #error
+       #elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
+       char *tmp = realpath((const char*)NULL, (char*)NULL);
+       #else
+       #error
+       #endif
+      ],
       [glibcxx_cv_realpath=yes],
       [glibcxx_cv_realpath=no])
   ])
   if test $glibcxx_cv_realpath = yes; then
-    AC_DEFINE(_GLIBCXX_USE_REALPATH, 1, [Define if realpath is available in <stdlib.h>.])
+    AC_DEFINE(_GLIBCXX_USE_REALPATH, 1, [Define if usable realpath is available in <stdlib.h>.])
   fi
   AC_MSG_RESULT($glibcxx_cv_realpath)
 dnl
diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc
index 016a78d..cfa44b1 100644
--- a/libstdc++-v3/src/filesystem/dir.cc
+++ b/libstdc++-v3/src/filesystem/dir.cc
@@ -22,6 +22,8 @@
 // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#define _XOPEN_SOURCE 700
+
 #include <experimental/filesystem>
 #include <utility>
 #include <stack>
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index cefb927..45daf34 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -22,6 +22,8 @@
 // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#define _XOPEN_SOURCE 700
+
 #include <experimental/filesystem>
 #include <functional>
 #include <stack>
@@ -96,23 +98,98 @@ namespace
 fs::path
 fs::canonical(const path& p, const path& base, error_code& ec)
 {
-  path can;
+  const path pa = absolute(p, base);
+  path result;
 #ifdef _GLIBCXX_USE_REALPATH
-  char* buffer = nullptr;
-#if defined(__SunOS_5_10) && defined(PATH_MAX)
-  buffer = (char*)::malloc(PATH_MAX);
-#endif
-  if (char_ptr rp = char_ptr{::realpath(absolute(p, base).c_str(), buffer)})
+  char_ptr buf{ nullptr };
+# if _XOPEN_VERSION < 700
+  // Not safe to call realpath(path, NULL)
+  buf.reset( (char*)::malloc(PATH_MAX) );
+# endif
+  if (char* rp = ::realpath(pa.c_str(), buf.get()))
     {
-      can.assign(rp.get());
+      if (buf == nullptr)
+	buf.reset(rp);
+      result.assign(rp);
       ec.clear();
+      return result;
+    }
+  if (errno != ENAMETOOLONG)
+    {
+      ec.assign(errno, std::generic_category());
+      return result;
     }
-  else
-    ec.assign(errno, std::generic_category());
-#else
-  ec = std::make_error_code(std::errc::not_supported);
 #endif
-  return can;
+
+  auto fail = [&ec, &result](int e) mutable {
+      if (!ec.value())
+	ec.assign(e, std::generic_category());
+      result.clear();
+  };
+
+  if (!exists(pa, ec))
+    {
+      fail(ENOENT);
+      return result;
+    }
+  // else we can assume no unresolvable symlink loops
+
+  result = pa.root_path();
+
+  deque<path> cmpts;
+  for (auto& f : pa.relative_path())
+    cmpts.push_back(f);
+
+  while (!cmpts.empty())
+    {
+      path f = std::move(cmpts.front());
+      cmpts.pop_front();
+
+      if (f.compare(".") == 0)
+	{
+	  if (!is_directory(result, ec))
+	    {
+	      fail(ENOTDIR);
+	      break;
+	    }
+	}
+      else if (f.compare("..") == 0)
+	{
+	  auto parent = result.parent_path();
+	  if (parent.empty())
+	    result = pa.root_path();
+	  else
+	    result.swap(parent);
+	}
+      else
+	{
+	  result /= f;
+
+	  if (is_symlink(result, ec))
+	    {
+	      path link = read_symlink(result, ec);
+	      if (!ec.value())
+		{
+		  if (link.is_absolute())
+		    {
+		      result = link.root_path();
+		      link = link.relative_path();
+		    }
+		  else
+		    result.remove_filename();
+
+		  cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		}
+	    }
+
+	  if (ec.value() || !exists(result, ec))
+	    {
+	      fail(ENOENT);
+	      break;
+	    }
+	}
+    }
+  return result;
 }
 
 fs::path
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
new file mode 100644
index 0000000..0bcb85b
--- /dev/null
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
@@ -0,0 +1,74 @@
+// Copyright (C) 2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++11 -lstdc++fs" }
+// { dg-require-filesystem-ts "" }
+
+#include <experimental/filesystem>
+#include <testsuite_hooks.h>
+#include <testsuite_fs.h>
+
+namespace fs = std::experimental::filesystem;
+
+void
+test01()
+{
+  std::error_code ec;
+  fs::path p = "";
+  canonical( p, ec );
+  VERIFY( !ec );
+
+  canonical( __gnu_test::nonexistent_path(), ec );
+  VERIFY( !ec );
+
+  p = "/";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( ec );
+
+  p = "/.";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( ec );
+
+  p = "/..";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( ec );
+
+  p = "/../.././.";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( ec );
+
+  p = "/dev/stdin";
+  if (exists(p))
+    {
+      auto p2 = canonical(p);
+      if (is_symlink(p))
+        VERIFY( p != p2 );
+      else
+        VERIFY( p == p2 );
+      VERIFY( canonical(p2) == p2 );
+    }
+}
+
+int
+main()
+{
+  test01();
+}

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 16/09/15 15:42 +0100, Jonathan Wakely wrote:
>@@ -22,6 +22,8 @@
> // see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> // <http://www.gnu.org/licenses/>.
> 
>+#define _XOPEN_SOURCE 700
>+
> #include <experimental/filesystem>
> #include <functional>
> #include <stack>

Unfortunately this completely breaks NetBSD, because defining
_XOPEN_SOURCE for this one file is inconsistent with how the autconf
test were run, so C99 functions that were implicitly available during
configure tests (because no _POSIX_C_SOURCE, _XOPEN_SOURCE etc. macro
was defined) are no longer available when compiling this translation
unit with _XOPEN_SOURCE defined.

Specifically, <wchar.h> in NetBSD 5.1 does:

#if !defined(_ANSI_SOURCE) && !defined(_POSIX_C_SOURCE) && \
    !defined(_XOPEN_SOURCE) && !defined(_NETBSD_SOURCE)
#define _NETBSD_SOURCE 1
#endif
...
#if defined(_ISOC99_SOURCE) || (__STDC_VERSION__ - 0) > 199901L || \
    defined(_NETBSD_SOURCE)
int vfwscanf(FILE * __restrict, const wchar_t * __restrict, _BSD_VA_LIST_);


So it's defined during configure and we define _GLIBCXX_HAVE_VFWSCANF
and our <cwchar> does:

#if _GLIBCXX_HAVE_VFWSCANF
  using ::vfwscanf;
#endif

but the value of _GLIBCXX_HAVE_VFWSCANF determined during configure is
not valid in src/filesystem/ops.cc after defining _XOPEN_SOURCE.

I don't know how to use _XOPEN_VERSION or _POSIX_VERSION to check for
a suitable realpath without defining one of those feature-test macros,
which then breaks other things.

(This is fixed in NetBSD 7.x which knows about C++11 and so defines
vfwscanf more liberally, but that doesn't help NetBSD 5.1).

Maybe I should just give up on realpath() and use my new
implementation everywhere :-(

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 16/09/15 17:02 +0100, Jonathan Wakely wrote:
>I don't know how to use _XOPEN_VERSION or _POSIX_VERSION to check for
>a suitable realpath without defining one of those feature-test macros,
>which then breaks other things.

I suppose we could also define _NETBSD_SOURCE manually, which is
basically what we do on GNU/Linux. G++ predefines _GNU_SOURCE so that
glibc gives us all declarations, but I want to move away from that and
stop polluting the global namespace with every GNU extension.

Maybe defining _NETBSD_SOURCE for versions older than 7.x is the right
solution though.

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

> It turns out I was wrong about BSD traditionally supporting
> realpath(path, NULL), it first appeared in these relatively recent
> versions:
>
> FreeBSD 9.0
> OpenBSD 5.0
> NetBSD 6.1
>
> Like Solaris 11, some of them still define _POSIX_VERSION=200112L even
> though they support passing NULL,  so we could hardcode the targets
> that are known to support it, but we'll still need a fallback for lots
> of slightly older targets anyway.
>
> So here's a new implementation of canonical() which only uses
> realpath() when this autoconf compile-or-link test passes:
>
> #if _XOPEN_VERSION < 500
> #error
> #elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
> char *tmp = realpath((const char*)NULL, (char*)NULL);
> #else
> #error
> #endif
>
> i.e. I use realpath() for Issue 7, or for Issue 6 if PATH_MAX is
> defined.

I probably wouldn't code the PATH_MAX test quite the same way.
I would expect it to be mainly Issue 6 implementations that don't
define the macro to want to provide the null extension since there
would otherwise be no safe way to use the function.

I didn't test it at all but I'd be inclined to write the conditional
to look more along these lines:

   #if _XOPEN_VERSION >= 700
     // Issue 7 and better -- null resolved_name means allocate
     char *tmp = realpath (file_name, (char*)NULL);
   #elif _XOPEN_VERSION == 600
      // Issue 6 -- null resolved_name is implementation-defined
   #  if FreeBSD 9.0 or OpenBSD 5.0 or NetBSD 6.1
     char *tmp = realpath (file_name, (char*)NULL);
   #  elif PATH_MAX
     char *tmp = realpath (file_name, malloc (PATH_MAX));
   #  else
   #    error No safe way to call realpath
   #  endif
   #elif _XOPEN_VERSION && PATH_MAX
     // SUSv2 -- null resolved_name is an error
     char *tmp = realpath (file_name, malloc (PATH_MAX));
   #else
   #  error realpath not supported or no safe way to call it
   #endif

>
> Then in filesystem::canonical() if _GLIBCXX_USE_REALPATH is set I use
> it, passing NULL for Issue 7, or allocating a buffer of PATH_MAX
> otherwise. If realpath isn't supported or fails with ENAMETOOLONG then
> I do the path traversal by hand (which can be done entirely using the
> std::experimental::filesystem operations).

FWIW, to work across filesystems with different _PC_PATH_MAX, I
suspect the operations might need to use readlinkat. I.e., they
might need to descend into each individual subdirectory to avoid
forming a temporary pathname that's too long for the file system
being traversed, even though both the initial and the final
pathnames are under the limit. (I haven't actually tested this
but I don't see where GLIBC handles this case so it might not
do the right thing either.)

>
> Only passing NULL for Issue 7 is quite conservative. It means we don't
> do it for targets that support it as an implementation-defined
> extension to Issue 6, which includes Solaris 11, the BSDs and even
> older GNU systems (including RHEL6). But that's OK, we have a fallback
> now so it means no loss of functionality, just efficiency.  We can
> tweak the config later for targets known to handle NULL.

Does the config test actually run? If not, I don't see the point
(it doesn't tell us anything the POSIX feature tests macros don't).
If it did run, it would fail since the first argument is null.

>
> The new testcase is not very thorough. I've run a few more involved
> tests that aren't suitable to check in until I figure out a good way
> of running filesystem tests that can create/remove arbitrary files and
> symlinks.

Yeah, writing good tests is always the hard part. If you need help
I can try to put some together in my spare time.

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 7401 bytes --]

On 16/09/15 11:36 -0600, Martin Sebor wrote:
>>It turns out I was wrong about BSD traditionally supporting
>>realpath(path, NULL), it first appeared in these relatively recent
>>versions:
>>
>>FreeBSD 9.0
>>OpenBSD 5.0
>>NetBSD 6.1
>>
>>Like Solaris 11, some of them still define _POSIX_VERSION=200112L even
>>though they support passing NULL,  so we could hardcode the targets
>>that are known to support it, but we'll still need a fallback for lots
>>of slightly older targets anyway.
>>
>>So here's a new implementation of canonical() which only uses
>>realpath() when this autoconf compile-or-link test passes:
>>
>>#if _XOPEN_VERSION < 500
>>#error
>>#elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
>>char *tmp = realpath((const char*)NULL, (char*)NULL);
>>#else
>>#error
>>#endif
>>
>>i.e. I use realpath() for Issue 7, or for Issue 6 if PATH_MAX is
>>defined.
>
>I probably wouldn't code the PATH_MAX test quite the same way.
>I would expect it to be mainly Issue 6 implementations that don't
>define the macro to want to provide the null extension since there
>would otherwise be no safe way to use the function.

OK.

>I didn't test it at all but I'd be inclined to write the conditional
>to look more along these lines:
>
>  #if _XOPEN_VERSION >= 700
>    // Issue 7 and better -- null resolved_name means allocate
>    char *tmp = realpath (file_name, (char*)NULL);
>  #elif _XOPEN_VERSION == 600
>     // Issue 6 -- null resolved_name is implementation-defined
>  #  if FreeBSD 9.0 or OpenBSD 5.0 or NetBSD 6.1
>    char *tmp = realpath (file_name, (char*)NULL);
>  #  elif PATH_MAX
>    char *tmp = realpath (file_name, malloc (PATH_MAX));
>  #  else
>  #    error No safe way to call realpath
>  #  endif
>  #elif _XOPEN_VERSION && PATH_MAX
>    // SUSv2 -- null resolved_name is an error
>    char *tmp = realpath (file_name, malloc (PATH_MAX));
>  #else
>  #  error realpath not supported or no safe way to call it
>  #endif

That would allow us to use realpath() on more targets, so would be a
good improvement, but I think it can wait for a later date. I think we
might also want two macros, USE_REALPATH and USE_REALPATH_NULL,
otherwise we just have to repeat the conditions in
src/filesystem/ops.cc to decide whether to use malloc() or not.

But I think the best solution for now is to not define any *_SOURCE
macros in the configure checks or in the source code (due to the
problem on NetBSD when _XOPEN_SOURCE is defined). If a new enough
_XOPEN_VERSION happens to be defined anyway (maybe due to an implicit
_GNU_SOURCE, or just a target where it's the default) then we'll use
realpath(), otherwise we use the fallback C++ implementation.

Here's a patch to do that, it's substantially the same as the last one
but doesn't define _XOPEN_SOURCE, and also tweaks some tests.

Tested powerpc64le-linux, x86_64-dragonfly4.1 and x86_64-netbsd5.1,
do you see any reason not to commit this for now?

Any improvements such as hardcoding checks for specific versions of
Solaris or the BSDs are QoI, and this is only an experimental TS, so I
don't want to spend the rest of stage 1 working on one function :-)


>>Then in filesystem::canonical() if _GLIBCXX_USE_REALPATH is set I use
>>it, passing NULL for Issue 7, or allocating a buffer of PATH_MAX
>>otherwise. If realpath isn't supported or fails with ENAMETOOLONG then
>>I do the path traversal by hand (which can be done entirely using the
>>std::experimental::filesystem operations).
>
>FWIW, to work across filesystems with different _PC_PATH_MAX, I
>suspect the operations might need to use readlinkat. I.e., they
>might need to descend into each individual subdirectory to avoid
>forming a temporary pathname that's too long for the file system
>being traversed, even though both the initial and the final
>pathnames are under the limit. (I haven't actually tested this
>but I don't see where GLIBC handles this case so it might not
>do the right thing either.)


Yes, I was planning to do it that way originally, then realised that
it can be done purely in C++ with the new filesystem API, which was
much easier!

It would be better to use openat() for each dir and use the *at()
functions, but I'd have to get my copy of Stevens out and read lots of
man pages, where as I already know the C++ API because I've recently
implemented the entire thing myself :-)

In an ideal world, with infinite time, it might be nice to create a
hybrid of the C++ Filesystem API and the *at() functions, even if only
for use internally in the library. It might reduce the number of
stat() calls, or at least the number of similar path traversals, if we
used openat(), fstatat() etc.

The Filesystem TS deals entirely in paths, because that's the only way
to be portable to both POSIX and Windows, but it would be really nice
to have a similar API based on file descriptors. It would certainly
make some things a lot more efficient.

A quick grep tells me that Boost.Filesystem doesn't use openat()
anywhere, and users have been happy with that implementation for many
years, so again I think improvements like this can wait (but feel free
to add something to Bugzilla suggesting it, or maybe a wiki page where
we can collect suggestions like this).

>>Only passing NULL for Issue 7 is quite conservative. It means we don't
>>do it for targets that support it as an implementation-defined
>>extension to Issue 6, which includes Solaris 11, the BSDs and even
>>older GNU systems (including RHEL6). But that's OK, we have a fallback
>>now so it means no loss of functionality, just efficiency.  We can
>>tweak the config later for targets known to handle NULL.
>
>Does the config test actually run? If not, I don't see the point
>(it doesn't tell us anything the POSIX feature tests macros don't).
>If it did run, it would fail since the first argument is null.

No, it's only a compile-or-link test, so only tells us if realpath()
is declared with a suitable signature, not that calling it works.
That's why I didn't bother passing a real argument, just used null
pointers of the right type.

We can't do config tests that need to be executed because you can't do
them for a cross-compiler, and we don't want the library config to
differ depending on whether you build native or cross.

>>
>>The new testcase is not very thorough. I've run a few more involved
>>tests that aren't suitable to check in until I figure out a good way
>>of running filesystem tests that can create/remove arbitrary files and
>>symlinks.
>
>Yeah, writing good tests is always the hard part. If you need help
>I can try to put some together in my spare time.

If you have suggestions for edge cases to test that would be great,
even if we can't actually test them yet.

My main obstacle to writing good tests right now is having some way to
create and destroy files safely in the tests. It's hard to test
functions like is_symlink() without first creating a symlink in a
known location, and also removing it again cleanly so the next
testsuite run doesn't fail if the file is already present.

One option would be to have libstdc++-v3/testsuite/Makefile create a
new sub-directory as a sandbox for filesystem tests, removing it if it
already exists. Then the tests can put anything they like in that new
dir without fear of trashing the user's files elsewhere on the FS!

That shouldn't be too hard to do, it's just a Simple Matter Of
Programming. And finding time.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 12539 bytes --]

commit ef25038796485298ff8f040bc79e0d9a371171fa
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 16 18:07:32 2015 +0100

    Implement filesystem::canonical() without realpath
    
    	PR libstdc++/67173
    	* acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check _XOPEN_VERSION
    	and PATH_MAX for _GLIBCXX_USE_REALPATH.
    	* config.h.in: Regenerate.
    	* configure: Regenerate.
    	* src/filesystem/ops.cc: (canonical) [!_GLIBCXX_USE_REALPATH]: Add
    	alternative implementation.
    	* testsuite/experimental/filesystem/operations/canonical.cc: New.
    	* testsuite/experimental/filesystem/operations/exists.cc: Add more
    	tests.
    	* testsuite/experimental/filesystem/operations/absolute.cc: Add test
    	variables.
    	* testsuite/experimental/filesystem/operations/copy.cc: Likewise.
    	* testsuite/experimental/filesystem/operations/current_path.cc:
    	Likewise.
    	* testsuite/experimental/filesystem/operations/file_size.cc: Likewise.
    	* testsuite/experimental/filesystem/operations/status.cc: Likewise.
    	* testsuite/experimental/filesystem/operations/temp_directory_path.cc:
    	Likewise.

diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 64c9b7e..c133c25 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -3947,13 +3947,24 @@ dnl
   AC_MSG_CHECKING([for realpath])
   AC_CACHE_VAL(glibcxx_cv_realpath, [dnl
     GCC_TRY_COMPILE_OR_LINK(
-      [#include <stdlib.h>],
-      [char *tmp = realpath((const char*)NULL, (char*)NULL);],
+      [
+       #include <stdlib.h>
+       #include <unistd.h>
+      ],
+      [
+       #if _XOPEN_VERSION < 500
+       #error
+       #elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
+       char *tmp = realpath((const char*)NULL, (char*)NULL);
+       #else
+       #error
+       #endif
+      ],
       [glibcxx_cv_realpath=yes],
       [glibcxx_cv_realpath=no])
   ])
   if test $glibcxx_cv_realpath = yes; then
-    AC_DEFINE(_GLIBCXX_USE_REALPATH, 1, [Define if realpath is available in <stdlib.h>.])
+    AC_DEFINE(_GLIBCXX_USE_REALPATH, 1, [Define if usable realpath is available in <stdlib.h>.])
   fi
   AC_MSG_RESULT($glibcxx_cv_realpath)
 dnl
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index cefb927..b5c8eb9 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -96,23 +96,98 @@ namespace
 fs::path
 fs::canonical(const path& p, const path& base, error_code& ec)
 {
-  path can;
+  const path pa = absolute(p, base);
+  path result;
 #ifdef _GLIBCXX_USE_REALPATH
-  char* buffer = nullptr;
-#if defined(__SunOS_5_10) && defined(PATH_MAX)
-  buffer = (char*)::malloc(PATH_MAX);
-#endif
-  if (char_ptr rp = char_ptr{::realpath(absolute(p, base).c_str(), buffer)})
+  char_ptr buf{ nullptr };
+# if _XOPEN_VERSION < 700
+  // Not safe to call realpath(path, NULL)
+  buf.reset( (char*)::malloc(PATH_MAX) );
+# endif
+  if (char* rp = ::realpath(pa.c_str(), buf.get()))
     {
-      can.assign(rp.get());
+      if (buf == nullptr)
+	buf.reset(rp);
+      result.assign(rp);
       ec.clear();
+      return result;
+    }
+  if (errno != ENAMETOOLONG)
+    {
+      ec.assign(errno, std::generic_category());
+      return result;
     }
-  else
-    ec.assign(errno, std::generic_category());
-#else
-  ec = std::make_error_code(std::errc::not_supported);
 #endif
-  return can;
+
+  auto fail = [&ec, &result](int e) mutable {
+      if (!ec.value())
+	ec.assign(e, std::generic_category());
+      result.clear();
+  };
+
+  if (!exists(pa, ec))
+    {
+      fail(ENOENT);
+      return result;
+    }
+  // else we can assume no unresolvable symlink loops
+
+  result = pa.root_path();
+
+  deque<path> cmpts;
+  for (auto& f : pa.relative_path())
+    cmpts.push_back(f);
+
+  while (!cmpts.empty())
+    {
+      path f = std::move(cmpts.front());
+      cmpts.pop_front();
+
+      if (f.compare(".") == 0)
+	{
+	  if (!is_directory(result, ec))
+	    {
+	      fail(ENOTDIR);
+	      break;
+	    }
+	}
+      else if (f.compare("..") == 0)
+	{
+	  auto parent = result.parent_path();
+	  if (parent.empty())
+	    result = pa.root_path();
+	  else
+	    result.swap(parent);
+	}
+      else
+	{
+	  result /= f;
+
+	  if (is_symlink(result, ec))
+	    {
+	      path link = read_symlink(result, ec);
+	      if (!ec.value())
+		{
+		  if (link.is_absolute())
+		    {
+		      result = link.root_path();
+		      link = link.relative_path();
+		    }
+		  else
+		    result.remove_filename();
+
+		  cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		}
+	    }
+
+	  if (ec.value() || !exists(result, ec))
+	    {
+	      fail(ENOENT);
+	      break;
+	    }
+	}
+    }
+  return result;
 }
 
 fs::path
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/absolute.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/absolute.cc
index 14625b5..f7507f5 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/absolute.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/absolute.cc
@@ -29,6 +29,8 @@ using std::experimental::filesystem::path;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   for (const path& p : __gnu_test::test_paths)
     VERIFY( absolute(p).is_absolute() );
 }
@@ -36,6 +38,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   path p1("/");
   VERIFY( absolute(p1) == p1 );
   VERIFY( absolute(p1, "/bar") == p1 );
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
new file mode 100644
index 0000000..d752feb
--- /dev/null
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
@@ -0,0 +1,77 @@
+// Copyright (C) 2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++11 -lstdc++fs" }
+// { dg-require-filesystem-ts "" }
+
+#include <experimental/filesystem>
+#include <testsuite_hooks.h>
+#include <testsuite_fs.h>
+
+namespace fs = std::experimental::filesystem;
+
+void
+test01()
+{
+  bool test __attribute__((unused)) = false;
+
+  std::error_code ec;
+  auto p = __gnu_test::nonexistent_path();
+  canonical( p, ec );
+  VERIFY( ec );
+
+  p = fs::current_path();
+  canonical( p, ec );
+  VERIFY( !ec );
+
+  p = "/";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( !ec );
+
+  p = "/.";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( !ec );
+
+  p = "/..";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( !ec );
+
+  p = "/../.././.";
+  p = canonical( p, ec );
+  VERIFY( p == "/" );
+  VERIFY( !ec );
+
+  p = "/dev/stdin";
+  if (exists(p))
+    {
+      auto p2 = canonical(p);
+      if (is_symlink(p))
+        VERIFY( p != p2 );
+      else
+        VERIFY( p == p2 );
+      VERIFY( canonical(p2) == p2 );
+    }
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc
index 2410c80..35d49f0 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc
@@ -29,6 +29,8 @@ using std::experimental::filesystem::path;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   for (const path& p : __gnu_test::test_paths)
     VERIFY( absolute(p).is_absolute() );
 }
@@ -36,6 +38,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   path p1("/");
   VERIFY( absolute(p1) == p1 );
   VERIFY( absolute(p1, "/bar") == p1 );
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/current_path.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/current_path.cc
index c242ac0..81ade73 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/current_path.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/current_path.cc
@@ -29,6 +29,8 @@ namespace fs = std::experimental::filesystem;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   fs::path dot(".");
   fs::path cwd = fs::current_path();
   std::error_code ec;
@@ -39,6 +41,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   auto oldwd = fs::current_path();
   auto tmpdir = fs::temp_directory_path();
   current_path(tmpdir);
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/exists.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/exists.cc
index 0f1e5aa..dba4a6f 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/exists.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/exists.cc
@@ -20,32 +20,37 @@
 
 #include <experimental/filesystem>
 #include <testsuite_hooks.h>
+#include <testsuite_fs.h>
 
 using std::experimental::filesystem::path;
 
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   VERIFY( exists(path{"/"}) );
   VERIFY( exists(path{"/."}) );
   VERIFY( exists(path{"."}) );
+  VERIFY( exists(path{".."}) );
+  VERIFY( exists(std::experimental::filesystem::current_path()) );
 }
 
 void
 test02()
 {
-  path rel{"xXxXx"};
-  while (exists(rel))
-    rel /= "x";
+  bool test __attribute__((unused)) = false;
+
+  path rel = __gnu_test::nonexistent_path();
   VERIFY( !exists(rel) );
 }
 
 void
 test03()
 {
-  path abs{"/xXxXx"};
-  while (exists(abs))
-    abs /= "x";
+  bool test __attribute__((unused)) = false;
+
+  path abs = absolute(__gnu_test::nonexistent_path());
   VERIFY( !exists(abs) );
 }
 
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/file_size.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/file_size.cc
index 04fa7bb..7603064 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/file_size.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/file_size.cc
@@ -27,6 +27,8 @@ namespace fs = std::experimental::filesystem;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   std::error_code ec;
   size_t size = fs::file_size(".", ec);
   VERIFY( ec == std::errc::is_a_directory );
@@ -45,6 +47,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   fs::path p = __gnu_test::nonexistent_path();
 
   std::error_code ec;
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/status.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/status.cc
index 2c54494..0f1730d 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/status.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/status.cc
@@ -27,6 +27,8 @@ namespace fs = std::experimental::filesystem;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   std::error_code ec;
   fs::file_status st1 = fs::status(".", ec);
   VERIFY( !ec );
@@ -39,6 +41,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   fs::path p = __gnu_test::nonexistent_path();
 
   std::error_code ec;
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc
index 2aacd1c..bd9b6ad 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc
@@ -37,6 +37,8 @@ namespace fs = std::experimental::filesystem;
 void
 test01()
 {
+  bool test __attribute__((unused)) = false;
+
   clean_env();
 
   if (!fs::exists("/tmp"))
@@ -53,6 +55,8 @@ test01()
 void
 test02()
 {
+  bool test __attribute__((unused)) = false;
+
   clean_env();
 
   if (::setenv("TMPDIR", __gnu_test::nonexistent_path().string().c_str(), 1))

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

> Tested powerpc64le-linux, x86_64-dragonfly4.1 and x86_64-netbsd5.1,
> do you see any reason not to commit this for now?

I see only a couple of potential problems: a missing test for
PATH_MAX in the unlikely event it's not defined (or is obscenely
large), and a missing check to avoid infinite loops due to symlinks.

>
> Any improvements such as hardcoding checks for specific versions of
> Solaris or the BSDs are QoI, and this is only an experimental TS, so I
> don't want to spend the rest of stage 1 working on one function :-)

Makes sense.

> My main obstacle to writing good tests right now is having some way to
> create and destroy files safely in the tests. It's hard to test
> functions like is_symlink() without first creating a symlink in a
> known location, and also removing it again cleanly so the next
> testsuite run doesn't fail if the file is already present.
>
> One option would be to have libstdc++-v3/testsuite/Makefile create a
> new sub-directory as a sandbox for filesystem tests, removing it if it
> already exists. Then the tests can put anything they like in that new
> dir without fear of trashing the user's files elsewhere on the FS!

I don't know how you feel about Tcl but writing a filesystem.exp
and adding a new "dg-fs" API would let each test can set up the
directory structure it needs.

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 16/09/15 16:04 -0600, Martin Sebor wrote:
>>Tested powerpc64le-linux, x86_64-dragonfly4.1 and x86_64-netbsd5.1,
>>do you see any reason not to commit this for now?
>
>I see only a couple of potential problems: a missing test for
>PATH_MAX in the unlikely event it's not defined (or is obscenely

In the current patch _GLIBCXX_USE_REALPATH won't be defined unless:

       #elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)

so if it's defined and _XOPEN_VERSION < 700 then we know PATH_MAX must
be defined (otherwise _GLIBCXX_USE_REALPATH wouldn't be).

>large), and a missing check to avoid infinite loops due to symlinks.

I thought about keeping track of where I'd been while expanding
symlinks, but then realised this will do it:

  if (!exists(pa, ec))
    {
      fail(ENOENT);
      return result;
    }
  // else we can assume no unresolvable symlink loops

If there's a symlink loop then exists(pa) will fail with ELOOP, and we
won't try to resolve it by hand.

And then after each step in the while(!cmpts.empty()) loop I also have
a check for !exists(result, ec), which should even handle the case
where the filesystem changes after the initial exists() call so that a
loop is introduced while we're canonicalising the path.


>>Any improvements such as hardcoding checks for specific versions of
>>Solaris or the BSDs are QoI, and this is only an experimental TS, so I
>>don't want to spend the rest of stage 1 working on one function :-)
>
>Makes sense.
>
>>My main obstacle to writing good tests right now is having some way to
>>create and destroy files safely in the tests. It's hard to test
>>functions like is_symlink() without first creating a symlink in a
>>known location, and also removing it again cleanly so the next
>>testsuite run doesn't fail if the file is already present.
>>
>>One option would be to have libstdc++-v3/testsuite/Makefile create a
>>new sub-directory as a sandbox for filesystem tests, removing it if it
>>already exists. Then the tests can put anything they like in that new
>>dir without fear of trashing the user's files elsewhere on the FS!
>
>I don't know how you feel about Tcl but writing a filesystem.exp
>and adding a new "dg-fs" API would let each test can set up the
>directory structure it needs.

My Tcl is very weak, but if that's the right approach then I can try
that.

Thanks again!

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 16/09/15 19:58 +0100, Jonathan Wakely wrote:
>commit ef25038796485298ff8f040bc79e0d9a371171fa
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Wed Sep 16 18:07:32 2015 +0100
>
>    Implement filesystem::canonical() without realpath
>    
>    	PR libstdc++/67173
>    	* acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check _XOPEN_VERSION
>    	and PATH_MAX for _GLIBCXX_USE_REALPATH.
>    	* config.h.in: Regenerate.
>    	* configure: Regenerate.
>    	* src/filesystem/ops.cc: (canonical) [!_GLIBCXX_USE_REALPATH]: Add
>    	alternative implementation.
>    	* testsuite/experimental/filesystem/operations/canonical.cc: New.
>    	* testsuite/experimental/filesystem/operations/exists.cc: Add more
>    	tests.
>    	* testsuite/experimental/filesystem/operations/absolute.cc: Add test
>    	variables.
>    	* testsuite/experimental/filesystem/operations/copy.cc: Likewise.
>    	* testsuite/experimental/filesystem/operations/current_path.cc:
>    	Likewise.
>    	* testsuite/experimental/filesystem/operations/file_size.cc: Likewise.
>    	* testsuite/experimental/filesystem/operations/status.cc: Likewise.
>    	* testsuite/experimental/filesystem/operations/temp_directory_path.cc:
>    	Likewise.

Committed to trunk.

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

On 09/16/2015 04:17 PM, Jonathan Wakely wrote:
> On 16/09/15 16:04 -0600, Martin Sebor wrote:
>>> Tested powerpc64le-linux, x86_64-dragonfly4.1 and x86_64-netbsd5.1,
>>> do you see any reason not to commit this for now?
>>
>> I see only a couple of potential problems: a missing test for
>> PATH_MAX in the unlikely event it's not defined (or is obscenely
>
> In the current patch _GLIBCXX_USE_REALPATH won't be defined unless:
>
>        #elif _XOPEN_VERSION >= 700 || defined(PATH_MAX)
>
> so if it's defined and _XOPEN_VERSION < 700 then we know PATH_MAX must
> be defined (otherwise _GLIBCXX_USE_REALPATH wouldn't be).
>
>> large), and a missing check to avoid infinite loops due to symlinks.
>
> I thought about keeping track of where I'd been while expanding
> symlinks, but then realised this will do it:
>
>   if (!exists(pa, ec))
>     {
>       fail(ENOENT);
>       return result;
>     }
>   // else we can assume no unresolvable symlink loops
>
> If there's a symlink loop then exists(pa) will fail with ELOOP, and we
> won't try to resolve it by hand.
>
> And then after each step in the while(!cmpts.empty()) loop I also have
> a check for !exists(result, ec), which should even handle the case
> where the filesystem changes after the initial exists() call so that a
> loop is introduced while we're canonicalising the path.

I obviously didn't read the patch carefully enough and missed
both the PATH_MAX check and the loop comment.

I see now the first exists test will detect symlink loops in
the original path. But I'm not convinced there isn't a corner
case that's subject to a TOCTOU race condition between the first
exists test and the while loop during which a symlink loop can
be introduced.

Suppose we call the function with /foo/bar as an argument and
the path exists and contains no symlinks. result is / and cmpts
is set to { foo, bar }. Just as the loop is entered, /foo/bar
is replaced with a symlink containing /foo/bar. The loop then
proceeds like so:

1. The first iteration removes foo from cmpts and sets result
to /foo. cmpts is { bar }.

2. The second iteration removes bar from cmpts, sets result to
/foo/bar, determines it's a symlink, reads its contents, sees
it's an absolute pathname and replaces result with /. It then
inserts the symlink's components { foo, bar } into cmpts. cmpts
becomes { foo, bar }. exists(result) succeeds.

3. The next iteration of the loop has the same initial state
as the first.

But I could have very easily missed something that takes care
of this corner case. If I did, sorry for the false alarm!

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 16/09/15 17:42 -0600, Martin Sebor wrote:
>I see now the first exists test will detect symlink loops in
>the original path. But I'm not convinced there isn't a corner
>case that's subject to a TOCTOU race condition between the first
>exists test and the while loop during which a symlink loop can
>be introduced.
>
>Suppose we call the function with /foo/bar as an argument and
>the path exists and contains no symlinks. result is / and cmpts
>is set to { foo, bar }. Just as the loop is entered, /foo/bar
>is replaced with a symlink containing /foo/bar. The loop then
>proceeds like so:
>
>1. The first iteration removes foo from cmpts and sets result
>to /foo. cmpts is { bar }.
>
>2. The second iteration removes bar from cmpts, sets result to
>/foo/bar, determines it's a symlink, reads its contents, sees
>it's an absolute pathname and replaces result with /. It then
>inserts the symlink's components { foo, bar } into cmpts. cmpts
>becomes { foo, bar }. exists(result) succeeds.
>
>3. The next iteration of the loop has the same initial state
>as the first.
>
>But I could have very easily missed something that takes care
>of this corner case. If I did, sorry for the false alarm!

No, you're right. The TS says such filesystem races are undefined:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4099.html#fs.race.behavior
but it would be nice to fail gracefully rather than DOS the
application.

The simplest approach would be to increment a counter every time we
follow a symlink, and if it reaches some limit decide something is
wrong and fail with ELOOP.

I don't see how anything else can be 100% bulletproof, because a truly
evil attacker could just keep altering the contents of symlinks so we
keep ping-ponging between two or more paths. If we keep track of paths
we've seen before the attacker could just keep changing the contents
to a unique path each time, that initially exists as a file, but by
the time we get to is_symlink() its become a symlink to a new path.

So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
<sys/param.h> the value the kernel uses? 20 seems quite low, I was
thinking of a much higher number.

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 17/09/15 12:16 +0100, Jonathan Wakely wrote:
>So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
><sys/param.h> the value the kernel uses? 20 seems quite low, I was
>thinking of a much higher number.

Until very recently Linux seemed to hardcode it to 40:
https://github.com/torvalds/linux/blob/v4.1/fs/namei.c#L865

That changed in v4.2 and I'm not sure what it does not, if this is the
relevant bit of code it uses MAXSYMLINKS:
https://github.com/torvalds/linux/blob/v4.2/fs/namei.c#L1647

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

On 17/09/15 12:16 +0100, Jonathan Wakely wrote:
>On 16/09/15 17:42 -0600, Martin Sebor wrote:
>>I see now the first exists test will detect symlink loops in
>>the original path. But I'm not convinced there isn't a corner
>>case that's subject to a TOCTOU race condition between the first
>>exists test and the while loop during which a symlink loop can
>>be introduced.
>>
>>Suppose we call the function with /foo/bar as an argument and
>>the path exists and contains no symlinks. result is / and cmpts
>>is set to { foo, bar }. Just as the loop is entered, /foo/bar
>>is replaced with a symlink containing /foo/bar. The loop then
>>proceeds like so:
>>
>>1. The first iteration removes foo from cmpts and sets result
>>to /foo. cmpts is { bar }.
>>
>>2. The second iteration removes bar from cmpts, sets result to
>>/foo/bar, determines it's a symlink, reads its contents, sees
>>it's an absolute pathname and replaces result with /. It then
>>inserts the symlink's components { foo, bar } into cmpts. cmpts
>>becomes { foo, bar }. exists(result) succeeds.
>>
>>3. The next iteration of the loop has the same initial state
>>as the first.
>>
>>But I could have very easily missed something that takes care
>>of this corner case. If I did, sorry for the false alarm!
>
>No, you're right. The TS says such filesystem races are undefined:
>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4099.html#fs.race.behavior
>but it would be nice to fail gracefully rather than DOS the
>application.
>
>The simplest approach would be to increment a counter every time we
>follow a symlink, and if it reaches some limit decide something is
>wrong and fail with ELOOP.
>
>I don't see how anything else can be 100% bulletproof, because a truly
>evil attacker could just keep altering the contents of symlinks so we
>keep ping-ponging between two or more paths. If we keep track of paths
>we've seen before the attacker could just keep changing the contents
>to a unique path each time, that initially exists as a file, but by
>the time we get to is_symlink() its become a symlink to a new path.
>
>So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
><sys/param.h> the value the kernel uses? 20 seems quite low, I was
>thinking of a much higher number.

This patch sets ELOOP after following 40 symlinks.

I can also move the exists(result, ec) check to the end, because the
is_symlink(result, ec) call will already check for existence on every
iteration that adds a component to the result.

I've also simplified the error handling (when exists(p, ec) fails it
sets ENOENT anyway) and moved !ec into the loop condition, rather than
using 'fail(e); break;' on errors.

I'm quite happy with this version now.



[-- Attachment #2: canonical.patch --]
[-- Type: text/plain, Size: 2481 bytes --]

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index b5c8eb9..95146bf 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -98,6 +98,7 @@ fs::canonical(const path& p, const path& base, error_code& ec)
 {
   const path pa = absolute(p, base);
   path result;
+
 #ifdef _GLIBCXX_USE_REALPATH
   char_ptr buf{ nullptr };
 # if _XOPEN_VERSION < 700
@@ -119,18 +120,9 @@ fs::canonical(const path& p, const path& base, error_code& ec)
     }
 #endif
 
-  auto fail = [&ec, &result](int e) mutable {
-      if (!ec.value())
-	ec.assign(e, std::generic_category());
-      result.clear();
-  };
-
   if (!exists(pa, ec))
-    {
-      fail(ENOENT);
-      return result;
-    }
-  // else we can assume no unresolvable symlink loops
+    return result;
+  // else: we know there are (currently) no unresolvable symlink loops
 
   result = pa.root_path();
 
@@ -138,18 +130,17 @@ fs::canonical(const path& p, const path& base, error_code& ec)
   for (auto& f : pa.relative_path())
     cmpts.push_back(f);
 
-  while (!cmpts.empty())
+  int max_allowed_symlinks = 40;
+
+  while (!cmpts.empty() && !ec)
     {
       path f = std::move(cmpts.front());
       cmpts.pop_front();
 
       if (f.compare(".") == 0)
 	{
-	  if (!is_directory(result, ec))
-	    {
-	      fail(ENOTDIR);
-	      break;
-	    }
+	  if (!is_directory(result, ec) && !ec)
+	    ec.assign(ENOTDIR, std::generic_category());
 	}
       else if (f.compare("..") == 0)
 	{
@@ -166,27 +157,30 @@ fs::canonical(const path& p, const path& base, error_code& ec)
 	  if (is_symlink(result, ec))
 	    {
 	      path link = read_symlink(result, ec);
-	      if (!ec.value())
+	      if (!ec)
 		{
-		  if (link.is_absolute())
+		  if (--max_allowed_symlinks == 0)
+		    ec.assign(ELOOP, std::generic_category());
+		  else
 		    {
-		      result = link.root_path();
-		      link = link.relative_path();
+		      if (link.is_absolute())
+			{
+			  result = link.root_path();
+			  link = link.relative_path();
+			}
+		      else
+			result.remove_filename();
+
+		      cmpts.insert(cmpts.begin(), link.begin(), link.end());
 		    }
-		  else
-		    result.remove_filename();
-
-		  cmpts.insert(cmpts.begin(), link.begin(), link.end());
 		}
 	    }
-
-	  if (ec.value() || !exists(result, ec))
-	    {
-	      fail(ENOENT);
-	      break;
-	    }
 	}
     }
+
+  if (ec || !exists(result, ec))
+    result.clear();
+
   return result;
 }
 

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On 16/09/15 23:50 +0100, Jonathan Wakely wrote:
>On 16/09/15 19:58 +0100, Jonathan Wakely wrote:
>>commit ef25038796485298ff8f040bc79e0d9a371171fa
>>Author: Jonathan Wakely <jwakely@redhat.com>
>>Date:   Wed Sep 16 18:07:32 2015 +0100
>>
>>   Implement filesystem::canonical() without realpath
>>   	PR libstdc++/67173
>>   	* acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check _XOPEN_VERSION
>>   	and PATH_MAX for _GLIBCXX_USE_REALPATH.
>>   	* config.h.in: Regenerate.
>>   	* configure: Regenerate.
>>   	* src/filesystem/ops.cc: (canonical) [!_GLIBCXX_USE_REALPATH]: Add
>>   	alternative implementation.
>>   	* testsuite/experimental/filesystem/operations/canonical.cc: New.
>>   	* testsuite/experimental/filesystem/operations/exists.cc: Add more
>>   	tests.
>>   	* testsuite/experimental/filesystem/operations/absolute.cc: Add test
>>   	variables.
>>   	* testsuite/experimental/filesystem/operations/copy.cc: Likewise.
>>   	* testsuite/experimental/filesystem/operations/current_path.cc:
>>   	Likewise.
>>   	* testsuite/experimental/filesystem/operations/file_size.cc: Likewise.
>>   	* testsuite/experimental/filesystem/operations/status.cc: Likewise.
>>   	* testsuite/experimental/filesystem/operations/temp_directory_path.cc:
>>   	Likewise.
>
>Committed to trunk.
>

I'm removing part of the new canonical.cc test as it fails
occasionally with:

terminate called after throwing an instance of 'std::experimental::filesystem::v1::__cxx11::filesystem_error'
  what():  filesystem error: cannot canonicalize: No such file or directory [/dev/stdin]
FAIL: experimental/filesystem/operations/canonical.cc execution test

This is odd, as I check with exists() before calling canonical(), but
rather than try to understand what is happening I'm just going to
remove that part.

Committed to trunk.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 972 bytes --]

commit a250423d1964952312bf97e6be3de987308a5164
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Sep 17 16:17:11 2015 +0100

    Remove non-deterministic part of canonical() test
    
    	* testsuite/experimental/filesystem/operations/canonical.cc: Remove
    	non-deterministic part of the test.

diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
index d752feb..5091a70 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/canonical.cc
@@ -57,17 +57,6 @@ test01()
   p = canonical( p, ec );
   VERIFY( p == "/" );
   VERIFY( !ec );
-
-  p = "/dev/stdin";
-  if (exists(p))
-    {
-      auto p2 = canonical(p);
-      if (is_symlink(p))
-        VERIFY( p != p2 );
-      else
-        VERIFY( p == p2 );
-      VERIFY( canonical(p2) == p2 );
-    }
 }
 
 int

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Martin Sebor]

On 09/17/2015 05:16 AM, Jonathan Wakely wrote:
> On 16/09/15 17:42 -0600, Martin Sebor wrote:
>> I see now the first exists test will detect symlink loops in
>> the original path. But I'm not convinced there isn't a corner
>> case that's subject to a TOCTOU race condition between the first
>> exists test and the while loop during which a symlink loop can
>> be introduced.
>>
>> Suppose we call the function with /foo/bar as an argument and
>> the path exists and contains no symlinks. result is / and cmpts
>> is set to { foo, bar }. Just as the loop is entered, /foo/bar
>> is replaced with a symlink containing /foo/bar. The loop then
>> proceeds like so:
>>
>> 1. The first iteration removes foo from cmpts and sets result
>> to /foo. cmpts is { bar }.
>>
>> 2. The second iteration removes bar from cmpts, sets result to
>> /foo/bar, determines it's a symlink, reads its contents, sees
>> it's an absolute pathname and replaces result with /. It then
>> inserts the symlink's components { foo, bar } into cmpts. cmpts
>> becomes { foo, bar }. exists(result) succeeds.
>>
>> 3. The next iteration of the loop has the same initial state
>> as the first.
>>
>> But I could have very easily missed something that takes care
>> of this corner case. If I did, sorry for the false alarm!
>
> No, you're right. The TS says such filesystem races are undefined:
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4099.html#fs.race.behavior
>
> but it would be nice to fail gracefully rather than DOS the
> application.
>
> The simplest approach would be to increment a counter every time we
> follow a symlink, and if it reaches some limit decide something is
> wrong and fail with ELOOP.
>
> I don't see how anything else can be 100% bulletproof, because a truly
> evil attacker could just keep altering the contents of symlinks so we
> keep ping-ponging between two or more paths. If we keep track of paths
> we've seen before the attacker could just keep changing the contents
> to a unique path each time, that initially exists as a file, but by
> the time we get to is_symlink() its become a symlink to a new path.
>
> So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
> <sys/param.h> the value the kernel uses? 20 seems quite low, I was
> thinking of a much higher number.

Yes, it is a corner case, and it's not really avoidable in the case
of hard links. For symlinks, POSIX defines the SYMLOOP_MAX constant
as the maximum, with the _SC_SYMLOOP_MAX and _PC_SYMLOOP_MAX
sysconf and pathconf variables. Otherwise 40 seems reasonable.

With this, I'll let you get back to work -- I think we've beat this
function to death ;)

Martin

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Andreas Schwab]

Jonathan Wakely <jwakely@redhat.com> writes:

> +  p = "/dev/stdin";
> +  if (exists(p))
> +    {
> +      auto p2 = canonical(p);
> +      if (is_symlink(p))
> +        VERIFY( p != p2 );
> +      else
> +        VERIFY( p == p2 );
> +      VERIFY( canonical(p2) == p2 );

This fails if stdin is a pipe, which doesn't have a (real) name, so
realpath fails.

$ echo | ./canonical.exe
terminate called after throwing an instance of 'std::experimental::filesystem::v1::__cxx11::filesystem_error'
  what():  filesystem error: cannot canonicalize: No such file or directory [/dev/stdin]

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

On 17/09/15 21:25 +0200, Andreas Schwab wrote:
>Jonathan Wakely <jwakely@redhat.com> writes:
>
>> +  p = "/dev/stdin";
>> +  if (exists(p))
>> +    {
>> +      auto p2 = canonical(p);
>> +      if (is_symlink(p))
>> +        VERIFY( p != p2 );
>> +      else
>> +        VERIFY( p == p2 );
>> +      VERIFY( canonical(p2) == p2 );
>
>This fails if stdin is a pipe, which doesn't have a (real) name, so
>realpath fails.
>
>$ echo | ./canonical.exe
>terminate called after throwing an instance of 'std::experimental::filesystem::v1::__cxx11::filesystem_error'
>  what():  filesystem error: cannot canonicalize: No such file or directory [/dev/stdin]

Ah, of course, the symlink exists but doesn't point to a real file.
Thanks for the explanation.

I'll re-add tests for symlinks when I come up with a proper method for
testing the Filesystem code.

Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

On 17/09/15 09:37 -0600, Martin Sebor wrote:
>On 09/17/2015 05:16 AM, Jonathan Wakely wrote:
>>On 16/09/15 17:42 -0600, Martin Sebor wrote:
>>>I see now the first exists test will detect symlink loops in
>>>the original path. But I'm not convinced there isn't a corner
>>>case that's subject to a TOCTOU race condition between the first
>>>exists test and the while loop during which a symlink loop can
>>>be introduced.
>>>
>>>Suppose we call the function with /foo/bar as an argument and
>>>the path exists and contains no symlinks. result is / and cmpts
>>>is set to { foo, bar }. Just as the loop is entered, /foo/bar
>>>is replaced with a symlink containing /foo/bar. The loop then
>>>proceeds like so:
>>>
>>>1. The first iteration removes foo from cmpts and sets result
>>>to /foo. cmpts is { bar }.
>>>
>>>2. The second iteration removes bar from cmpts, sets result to
>>>/foo/bar, determines it's a symlink, reads its contents, sees
>>>it's an absolute pathname and replaces result with /. It then
>>>inserts the symlink's components { foo, bar } into cmpts. cmpts
>>>becomes { foo, bar }. exists(result) succeeds.
>>>
>>>3. The next iteration of the loop has the same initial state
>>>as the first.
>>>
>>>But I could have very easily missed something that takes care
>>>of this corner case. If I did, sorry for the false alarm!
>>
>>No, you're right. The TS says such filesystem races are undefined:
>>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4099.html#fs.race.behavior
>>
>>but it would be nice to fail gracefully rather than DOS the
>>application.
>>
>>The simplest approach would be to increment a counter every time we
>>follow a symlink, and if it reaches some limit decide something is
>>wrong and fail with ELOOP.
>>
>>I don't see how anything else can be 100% bulletproof, because a truly
>>evil attacker could just keep altering the contents of symlinks so we
>>keep ping-ponging between two or more paths. If we keep track of paths
>>we've seen before the attacker could just keep changing the contents
>>to a unique path each time, that initially exists as a file, but by
>>the time we get to is_symlink() its become a symlink to a new path.
>>
>>So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
>><sys/param.h> the value the kernel uses? 20 seems quite low, I was
>>thinking of a much higher number.
>
>Yes, it is a corner case, and it's not really avoidable in the case
>of hard links. For symlinks, POSIX defines the SYMLOOP_MAX constant
>as the maximum, with the _SC_SYMLOOP_MAX and _PC_SYMLOOP_MAX
>sysconf and pathconf variables. Otherwise 40 seems reasonable.
>
>With this, I'll let you get back to work -- I think we've beat this
>function to death ;)

Here's what I committed. Similar to the last patch, but using the new
is_dot and is_dotdot helpers.



[-- Attachment #2: patch-fs-3.txt --]
[-- Type: text/plain, Size: 2925 bytes --]

commit 8128173a00c234ccf34e258115747fa0e3b4457a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 23 02:00:57 2015 +0100

    Limit number of symlinks that canonical() will resolve
    
    	* src/filesystem/ops.cc (canonical): Simplify error handling and
    	limit number of symlinks that can be resolved.

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 5ff8120..7b261fb 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -116,6 +116,7 @@ fs::canonical(const path& p, const path& base, error_code& ec)
 {
   const path pa = absolute(p, base);
   path result;
+
 #ifdef _GLIBCXX_USE_REALPATH
   char_ptr buf{ nullptr };
 # if _XOPEN_VERSION < 700
@@ -137,18 +138,9 @@ fs::canonical(const path& p, const path& base, error_code& ec)
     }
 #endif
 
-  auto fail = [&ec, &result](int e) mutable {
-      if (!ec.value())
-	ec.assign(e, std::generic_category());
-      result.clear();
-  };
-
   if (!exists(pa, ec))
-    {
-      fail(ENOENT);
-      return result;
-    }
-  // else we can assume no unresolvable symlink loops
+    return result;
+  // else: we know there are (currently) no unresolvable symlink loops
 
   result = pa.root_path();
 
@@ -156,20 +148,19 @@ fs::canonical(const path& p, const path& base, error_code& ec)
   for (auto& f : pa.relative_path())
     cmpts.push_back(f);
 
-  while (!cmpts.empty())
+  int max_allowed_symlinks = 40;
+
+  while (!cmpts.empty() && !ec)
     {
       path f = std::move(cmpts.front());
       cmpts.pop_front();
 
-      if (f.compare(".") == 0)
+      if (is_dot(f))
 	{
-	  if (!is_directory(result, ec))
-	    {
-	      fail(ENOTDIR);
-	      break;
-	    }
+	  if (!is_directory(result, ec) && !ec)
+	    ec.assign(ENOTDIR, std::generic_category());
 	}
-      else if (f.compare("..") == 0)
+      else if (is_dotdot(f))
 	{
 	  auto parent = result.parent_path();
 	  if (parent.empty())
@@ -184,27 +175,30 @@ fs::canonical(const path& p, const path& base, error_code& ec)
 	  if (is_symlink(result, ec))
 	    {
 	      path link = read_symlink(result, ec);
-	      if (!ec.value())
+	      if (!ec)
 		{
-		  if (link.is_absolute())
-		    {
-		      result = link.root_path();
-		      link = link.relative_path();
-		    }
+		  if (--max_allowed_symlinks == 0)
+		    ec.assign(ELOOP, std::generic_category());
 		  else
-		    result.remove_filename();
+		    {
+		      if (link.is_absolute())
+			{
+			  result = link.root_path();
+			  link = link.relative_path();
+			}
+		      else
+			result.remove_filename();
 
-		  cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		      cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		    }
 		}
 	    }
-
-	  if (ec.value() || !exists(result, ec))
-	    {
-	      fail(ENOENT);
-	      break;
-	    }
 	}
     }
+
+  if (ec || !exists(result, ec))
+    result.clear();
+
   return result;
 }