Boost C++ Libraries: Ticket #5652: recursive_directory_iterator fails on cyclic symbolic links

attachment set

Sat, 15 Oct 2011 19:28:52 GMT

attachment → bug5652.diff

Fix for bug 5652: Check if file is symbolic link before stat:ing it during recursion (an not the other way around)

Sat, 15 Oct 2011 19:32:32 GMT

I can confirm this issue and there is an easy fix. To reproduce and test the issue the following simple program that counts the files (recursively) in a directory can be used on a directory containing the "symlink to self" as described by Geurt.

#include <string> #include <iostream>

#include <boost/filesystem.hpp>

int ftw(const std::string &root) {

using namespace boost::filesystem; typedef recursive_directory_iterator walker; int count = 0; for (walker w(root); w != walker(); ++w)

++count;

return count;

}

int main (int argc, const char argv) {

int count = ftw(argv[1]); std::cout << count << std::endl;

}

The problem is in the increment method in recur_dir_itr_imp. It checks the file with is_directory, which will end up in a "stat" call on the file. Of course if the file is a symlink to itself stat will try to follow the link indefinitely (since it will try to deference the symlink until a real file is found). This causes stat to generate an ELOOP error and the exception is thrown.

The solution is to not check if the file is a directory if it is a symlink - unless of course we are following symlinks during recursion, in which case the exception should occur. We can test this by adding symlink_option::recurse as an option to the walker constructor in the above example.

cc set

Sat, 15 Oct 2011 19:41:59 GMT

cc macbishop@… added

For efficiency reasons it would also be a good idea to first check for symlink_option::recurse and *then* for is_symlink, since if we are following symlinks we will not have to do the (expensive) call to lstat and we will only do an integer comparision.

attachment set

Sat, 15 Oct 2011 19:45:08 GMT

attachment → bug5652-perf-opt.diff

Performance optimized patch for bug 5652: Check if file is symbolic link before stat:ing it during recursion (an not the other way around) but first check if we are following symlinks

benjamin.kircher@… — Sun, 15 Jan 2012 20:03:10 GMT

Just walked into this with 1.48. +1

status changed

Beman Dawes — Tue, 17 Jan 2012 14:36:44 GMT

status new → assigned

Replying to Daniel Aarno <macbishop@…>:

I can confirm this issue and there is an easy fix. To reproduce and test the issue the following simple program that counts the files (recursively) in a directory can be used on a directory containing the "symlink to self" as described by Geurt.

#include <string> #include <iostream>

#include <boost/filesystem.hpp>

int ftw(const std::string &root) {

using namespace boost::filesystem; typedef recursive_directory_iterator walker; int count = 0; for (walker w(root); w != walker(); ++w)

++count;

return count;

}

int main (int argc, const char argv) {

int count = ftw(argv[1]); std::cout << count << std::endl;

}

Thanks for the test program! Providing a test program is a very effective way to communicative exactly what problem you are having, and prevents me from chasing my tail around in circles.

On Windows the program throws with a message "The name of the file cannot be resolved by the system". I haven't looked at your patch yet, but wanted to thank you for the test case first,

--Beman

macbishop@… — Tue, 17 Jan 2012 14:51:50 GMT

Replying to bemandawes:

On Windows the program throws with a message "The name of the file cannot be resolved by the system". I haven't looked at your patch yet, but wanted to thank you for the test case first

Thanks for looking into this. To clarify, the test case takes as its first argument a path to a directory containing the recursive symlink, so you need to:

1) Setup a directory with a symlink that points to itself (not sure how to do it on windows) 2) Then run the test case, with the path to the directory given as the only argument, e.g. ./a.out thedir

Let me know if you have any questions or suggestions on the test-case or patches.

status, milestone changed; resolution set

Beman Dawes — Sun, 22 Jan 2012 16:14:05 GMT

status assigned → closed
resolution → fixed
milestone To Be Determined → Boost 1.49.0

Fixed by changeset 76556.

Thanks, all!

--Beman