This is the mail archive of the mailing list for the libstdc++ project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]