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

[Jonathan Wakely <jwakely@redhat.com>]

[-- 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))