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

[Jonathan Wakely]

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
 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.


commit 8128173a00c234ccf34e258115747fa0e3b4457a
Author: Jonathan Wakely 
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(

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

[Jonathan Wakely]

On 17/09/15 21:25 +0200, Andreas Schwab wrote:

Jonathan Wakely  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.

[Andreas Schwab]

Jonathan Wakely  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, sch...@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.

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

[Jonathan Wakely]

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 
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.


commit a250423d1964952312bf97e6be3de987308a5164
Author: Jonathan Wakely 
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.

[Jonathan Wakely]

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
 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.


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();
+   }
+

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
 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]

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
 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.

[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 19:58 +0100, Jonathan Wakely wrote:

commit ef25038796485298ff8f040bc79e0d9a371171fa
Author: Jonathan Wakely 
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.

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

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

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]

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.

[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
// .

+#define _XOPEN_SOURCE 700
+
#include 
#include 
#include 


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,  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  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 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.


commit e79ad2dbcb14d1e66f6edead4ff87b62e575a8e7
Author: Jonathan Wakely 
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_CPLU

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.

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

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