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

[Martin Sebor <msebor@gmail.com>]

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