Harden RecursiveDirectoryIterator when walking the filesystem. (#2001)

* In the implementation for the *Traverse strategies the next method performs an unguarded list directory.  If the directory is not accessible an unrecoverable error is raised thus ruining the walk.  This changeset adopts and adapts the error handling protocol as defined in Python's os.walk function where errors from listdir are ignored or are reported to an optional on error callback function.

* Expand DirectoryIteratorsTest testsuite to confirm the hardened iterator behaviour over unreadable directories.

* Expand DirectoryIteratorsTest testsuite to confirm the hardened iterator behaviour over
  unreadable directories.  Correct bad formatting

* fix clang compile

Foundation/Foundation_x64_vs140.vcxproj.filters

@@ -207,9 +207,6 @@
     <ClCompile Include="src\Debugger.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
-    <ClCompile Include="src\DirectoryIteratorStrategy.cpp">
-      <Filter>Core\Source Files</Filter>
-    </ClCompile>
     <ClCompile Include="src\Environment.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
@@ -267,9 +264,6 @@
     <ClCompile Include="src\RefCountedObject.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
-    <ClCompile Include="src\SortedDirectoryIterator.cpp">
-      <Filter>Core\Source Files</Filter>
-    </ClCompile>
     <ClCompile Include="src\String.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
@@ -894,6 +888,12 @@
     <ClCompile Include="src\Checksum64.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="src\DirectoryIteratorStrategy.cpp">
+      <Filter>Filesystem\Source Files</Filter>
+    </ClCompile>
+    <ClCompile Include="src\SortedDirectoryIterator.cpp">
+      <Filter>Filesystem\Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="include\Poco\Any.h">
@@ -1037,9 +1037,6 @@
     <ClInclude Include="include\Poco\SingletonHolder.h">
       <Filter>Core\Header Files</Filter>
     </ClInclude>
-    <ClInclude Include="include\Poco\SortedDirectoryIterator.h">
-      <Filter>Core\Header Files</Filter>
-    </ClInclude>
     <ClInclude Include="include\Poco\String.h">
       <Filter>Core\Header Files</Filter>
     </ClInclude>
@@ -1679,9 +1676,6 @@
     <ClInclude Include="include\Poco\Delegate.h">
       <Filter>Events\Header Files</Filter>
     </ClInclude>
-    <ClInclude Include="include\Poco\DirectoryIteratorStrategy.h">
-      <Filter>Events\Header Files</Filter>
-    </ClInclude>
     <ClInclude Include="include\Poco\EventArgs.h">
       <Filter>Events\Header Files</Filter>
     </ClInclude>
@@ -1835,6 +1829,12 @@
     <ClInclude Include="include\Poco\MakeUnique.h">
       <Filter>Core\Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="include\Poco\SortedDirectoryIterator.h">
+      <Filter>Filesystem\Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="include\Poco\DirectoryIteratorStrategy.h">
+      <Filter>Filesystem\Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="src\pocomsg.rc">

Foundation/include/Poco/DirectoryIteratorStrategy.h

@@ -20,6 +20,8 @@
 
 #include "Poco/Foundation.h"
 #include "Poco/DirectoryIterator.h"
+#include "Poco/BasicEvent.h"
+#include "Poco/EventArgs.h"
 #include <stack>
 #include <queue>
 #include <functional>
@@ -39,6 +41,8 @@ public:
 		D_INFINITE = 0 /// Special value for infinite traverse depth.
 	};
 
+	Poco::BasicEvent<const std::string> traverseError;
+
 	TraverseBase(DepthFunPtr depthDeterminer, UInt16 maxDepth = D_INFINITE);
 
 protected:

Foundation/include/Poco/RecursiveDirectoryIterator.h

@@ -135,6 +135,18 @@ public:
 		return _pImpl->maxDepth();
 	}
 
+	template <typename T>
+	void onError(T& obj, void (T::*pCB)(const void*, const std::string&))
+		/// Binds the event to the given method.
+		///
+		/// The callback method will be called if the Traverse class fails
+		/// to read a directory. 
+		///
+		/// Usage:
+		///     onError(*this, &MyClass::myCallback);
+	{
+		_pImpl->template onError<T>(obj, pCB);
+	}
 
 	MyType& operator = (const MyType& it)
 	{

Foundation/include/Poco/RecursiveDirectoryIteratorImpl.h

@@ -20,6 +20,7 @@
 
 #include "Poco/Foundation.h"
 #include "Poco/DirectoryIteratorStrategy.h"
+#include "Poco/Delegate.h"
 #include <stack>
 #include <functional>
 
@@ -76,6 +77,13 @@ public:
 	{
 		return _current;
 	}
+
+	template <typename T>
+	void onError(T& obj, void (T::*pCB)(const void*, const std::string&))
+	{
+		_traverseStrategy.traverseError += delegate(&obj, pCB);
+	}
+
 	const std::string& next()
 	{
 		if (_isFinished)

Foundation/src/DirectoryIteratorStrategy.cpp

@@ -21,8 +21,8 @@ namespace Poco {
 //
 // TraverseBase
 //
-TraverseBase::TraverseBase(DepthFunPtr depthDeterminer, UInt16 maxDepth)
-	: _depthDeterminer(depthDeterminer), _maxDepth(maxDepth)
+TraverseBase::TraverseBase(DepthFunPtr depthDeterminer, UInt16 maxDepth): _depthDeterminer(depthDeterminer), 
+	_maxDepth(maxDepth)
 {
 }
 
@@ -49,8 +49,8 @@ bool TraverseBase::isDirectory(Poco::File& file)
 //
 // ChildrenFirstTraverse
 //
-ChildrenFirstTraverse::ChildrenFirstTraverse(DepthFunPtr depthDeterminer, UInt16 maxDepth)
-	: TraverseBase(depthDeterminer, maxDepth)
+ChildrenFirstTraverse::ChildrenFirstTraverse(DepthFunPtr depthDeterminer, UInt16 maxDepth):
+	TraverseBase(depthDeterminer, maxDepth)
 {
 }
 
@@ -61,16 +61,11 @@ const std::string ChildrenFirstTraverse::next(Stack* itStack, bool* isFinished)
 	poco_check_ptr(isFinished);
 	poco_assert(!(*isFinished));
 
-	std::stack<DirectoryIterator> it;
-
-	//_depthDeterminer(it);
-
 	// go deeper into not empty directory
 	// (if depth limit allows)
 	bool isDepthLimitReached = isFiniteDepth() && _depthDeterminer(*itStack) >= _maxDepth;
 	if (!isDepthLimitReached && isDirectory(*itStack->top()))
 	{
-		// check the dir is iterable
 		try
 		{
 			DirectoryIterator child_it(itStack->top().path());
@@ -83,6 +78,8 @@ const std::string ChildrenFirstTraverse::next(Stack* itStack, bool* isFinished)
 		}
 		catch (...)
 		{
+			// Failed to iterate child dir.
+			traverseError.notify(this, itStack->top()->path());
 		}
 	}
 
@@ -145,24 +142,23 @@ const std::string SiblingsFirstTraverse::next(Stack* itStack, bool* isFinished)
 		{
 			std::string dir = _dirsStack.top().front();
 			_dirsStack.top().pop();
-			DirectoryIterator child_it;
-			
-			// check the dir is iterable
+
 			try
 			{
-				child_it = dir;
+				DirectoryIterator child_it(dir);
+
+				// check if directory is empty
+				if (child_it != _itEnd)
+				{
+					itStack->push(child_it);
+					_dirsStack.push(std::queue<std::string>());
+					return child_it->path();
+				}
 			}
 			catch (...)
 			{
-				continue;
-			}
-
-			// check if directory is empty
-			if (child_it != _itEnd)
-			{
-				itStack->push(child_it);
-				_dirsStack.push(std::queue<std::string>());
-				return child_it->path();
+				// Failed to iterate child dir.
+				traverseError.notify(this, dir);
 			}
 		}
 

Foundation/testsuite/TestLibrary_x64_vs140.vcxproj.filters

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup>
     <Filter Include="Source Files">

Foundation/testsuite/TestSuite_x64_vs140.vcxproj.filters

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup>
     <Filter Include="Core">
@@ -201,9 +201,6 @@
     <ClCompile Include="src\CoreTestSuite.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
-    <ClCompile Include="src\DirectoryIteratorsTest.cpp">
-      <Filter>Core\Source Files</Filter>
-    </ClCompile>
     <ClCompile Include="src\DynamicFactoryTest.cpp">
       <Filter>Core\Source Files</Filter>
     </ClCompile>
@@ -606,6 +603,9 @@
     <ClCompile Include="src\VarTest.cpp">
       <Filter>Dynamic\Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="src\DirectoryIteratorsTest.cpp">
+      <Filter>Filesystem\Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="src\AnyTest.h">
@@ -629,9 +629,6 @@
     <ClInclude Include="src\CoreTestSuite.h">
       <Filter>Core\Header Files</Filter>
     </ClInclude>
-    <ClInclude Include="src\DirectoryIteratorsTest.h">
-      <Filter>Core\Header Files</Filter>
-    </ClInclude>
     <ClInclude Include="src\DynamicAnyTest.h">
       <Filter>Core\Header Files</Filter>
     </ClInclude>
@@ -1034,5 +1031,8 @@
     <ClInclude Include="src\VarTest.h">
       <Filter>Dynamic\Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="src\DirectoryIteratorsTest.h">
+      <Filter>Filesystem\Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
-</Project>
+</Project>

Foundation/testsuite/src/BasicEventTest.cpp

@@ -35,6 +35,7 @@ BasicEventTest::~BasicEventTest()
 {
 }
 
+
 void BasicEventTest::testNoDelegate()
 {
 	int tmp = 0;
@@ -113,9 +114,11 @@ void BasicEventTest::testNoDelegate()
 	Void -= delegate(BasicEventTest::onStaticVoid);
 }
 
+
 void BasicEventTest::testSingleDelegate()
 {
 	int tmp = 0;
+	std::string tmpStr;
 	EventArgs args;
 
 	assert (_count == 0);
@@ -131,7 +134,17 @@ void BasicEventTest::testSingleDelegate()
 	ConstSimple += delegate(this, &BasicEventTest::onConstSimple);
 	ConstSimple.notify(this, tmp);
 	assert (_count == 3);
-	
+
+	tmpStr = "str";
+	SimpleString += delegate(this, &BasicEventTest::onString);
+	SimpleString.notify(this, tmpStr);
+	assert(_str == "str");
+
+	tmpStr = "strConst";
+	ConstString += delegate(this, &BasicEventTest::onConstString);
+	ConstString.notify(this, tmpStr);
+	assert(_str == "strConst");
+
 	EventArgs* pArgs = &args;
 	Complex += delegate(this, &BasicEventTest::onComplex);
 	Complex.notify(this, pArgs);
@@ -155,6 +168,7 @@ void BasicEventTest::testSingleDelegate()
 	
 }
 
+
 void BasicEventTest::testDuplicateRegister()
 {
 	int tmp = 0;
@@ -212,6 +226,7 @@ void BasicEventTest::testDuplicateUnregister()
 	assert (_count == 1);
 }
 
+
 void BasicEventTest::testDisabling()
 {
 	int tmp = 0;
@@ -234,6 +249,7 @@ void BasicEventTest::testDisabling()
 	assert (_count == 1);
 }
 
+
 void BasicEventTest::testExpire()
 {
 	int tmp = 0;
@@ -258,6 +274,7 @@ void BasicEventTest::testExpire()
 	assert (_count == 4);
 }
 
+
 void BasicEventTest::testExpireReRegister()
 {
 	int tmp = 0;
@@ -280,6 +297,7 @@ void BasicEventTest::testExpireReRegister()
 	assert (_count == 3);
 }
 
+
 void BasicEventTest::testReturnParams()
 {
 	DummyDelegate o1;
@@ -290,6 +308,7 @@ void BasicEventTest::testReturnParams()
 	assert (tmp == 1);
 }
 
+
 void BasicEventTest::testOverwriteDelegate()
 {
 	DummyDelegate o1;
@@ -301,6 +320,7 @@ void BasicEventTest::testOverwriteDelegate()
 	assert (tmp == 2);
 }
 
+
 void BasicEventTest::testAsyncNotify()
 {
 	Poco::BasicEvent<int>* pSimple= new Poco::BasicEvent<int>();
@@ -338,11 +358,13 @@ void BasicEventTest::onStaticVoid(const void* pSender)
 	p->_count++;
 }
 
+
 void BasicEventTest::onVoid(const void* pSender)
 {
 	_count++;
 }
 
+
 void BasicEventTest::onSimpleNoSender(int& i)
 {
 	_count++;
@@ -379,42 +401,62 @@ void BasicEventTest::onSimpleOther(const void* pSender, int& i)
 	_count+=100;
 }
 
+
 void BasicEventTest::onConstSimple(const void* pSender, const int& i)
 {
 	_count++;
 }
 
+
+void BasicEventTest::onString(const void* pSender, std::string& str)
+{
+	_str = str;
+}
+
+
+void BasicEventTest::onConstString(const void* pSender, const std::string& str)
+{
+	_str = str;
+}
+
+
 void BasicEventTest::onComplex(const void* pSender, Poco::EventArgs* & i)
 {
 	_count++;
 }
 
+
 void BasicEventTest::onComplex2(const void* pSender, Poco::EventArgs & i)
 {
 	_count++;
 }
 
+
 void BasicEventTest::onConstComplex(const void* pSender, const Poco::EventArgs*& i)
 {
 	_count++;
 }
 
+
 void BasicEventTest::onConst2Complex(const void* pSender, const Poco::EventArgs * const & i)
 {
 	_count++;
 }
 
+
 void BasicEventTest::onAsync(const void* pSender, int& i)
 {
 	Poco::Thread::sleep(700);
 	_count += LARGEINC ;
 }
 
+
 int BasicEventTest::getCount() const
 {
 	return _count;
 }
 
+
 void BasicEventTest::setUp()
 {
 	_count = 0;

Foundation/testsuite/src/BasicEventTest.h

@@ -24,11 +24,14 @@ class BasicEventTest: public CppUnit::TestCase
 {
 	Poco::BasicEvent<void> Void;
 	Poco::BasicEvent<int> Simple;
+	Poco::BasicEvent<std::string> SimpleString;
+	Poco::BasicEvent<const std::string> ConstString;
 	Poco::BasicEvent<const int> ConstSimple;
 	Poco::BasicEvent<Poco::EventArgs*> Complex;
 	Poco::BasicEvent<Poco::EventArgs> Complex2;
 	Poco::BasicEvent<const Poco::EventArgs*> ConstComplex;
 	Poco::BasicEvent<const Poco::EventArgs * const> Const2Complex;
+
 public:
 	BasicEventTest(const std::string& name);
 	~BasicEventTest();
@@ -64,6 +67,8 @@ protected:
 	void onSimple(const void* pSender, int& i);
 	void onSimpleOther(const void* pSender, int& i);
 	void onConstSimple(const void* pSender, const int& i);
+	void onString(const void* pSender, std::string& i);
+	void onConstString(const void* pSender, const std::string& i);
 	void onComplex(const void* pSender, Poco::EventArgs* & i);
 	void onComplex2(const void* pSender, Poco::EventArgs & i);
 	void onConstComplex(const void* pSender, const Poco::EventArgs*& i);
@@ -72,7 +77,8 @@ protected:
 
 	int getCount() const;
 private:
-	int		_count;
+	int _count;
+	std::string _str;
 };
 
 

Foundation/testsuite/src/DirectoryIteratorsTest.cpp

@@ -15,14 +15,75 @@
 #include "Poco/SortedDirectoryIterator.h"
 #include "Poco/RecursiveDirectoryIterator.h"
 #include "Poco/FileStream.h"
+#include "Poco/Exception.h"
 
 #include <iostream>
 
+#if defined(POCO_OS_FAMILY_UNIX)
+#include <errno.h>
+#include <sys/stat.h>
+#endif
+
 using namespace Poco;
 
 
-DirectoryIteratorsTest::DirectoryIteratorsTest(const std::string& rName):
-	CppUnit::TestCase(rName)
+#if defined(POCO_OS_FAMILY_UNIX)
+
+static void setReadable(const std::string& path, bool flag)
+{
+	poco_assert(!path.empty());
+
+	struct stat st;
+	if (stat(path.c_str(), &st) != 0)
+	{
+		throw FileException("stat error", path, errno);
+	}
+	mode_t mode;
+	if (flag)
+	{
+		mode = st.st_mode | S_IRUSR | S_IRGRP | S_IROTH;
+	}
+	else
+	{
+		mode_t rmask = S_IRUSR | S_IRGRP | S_IROTH;
+		mode = st.st_mode & ~rmask;
+	}
+	if (chmod(path.c_str(), mode) != 0)
+	{
+		throw FileException("chmod", path, errno);
+	}
+}
+
+#else
+
+static void setReadable(const std::string& path, bool flag)
+{
+	poco_assert(!path.empty());
+}
+
+#endif
+
+
+class TemporarilyHidePath
+{
+public:
+	TemporarilyHidePath(std::string path)
+		: _path(path)
+	{
+		setReadable(_path, false);
+	}
+
+	~TemporarilyHidePath()
+	{
+		setReadable(_path, true);
+	}
+private:
+	std::string _path;
+};
+
+
+DirectoryIteratorsTest::DirectoryIteratorsTest(const std::string& name):
+	CppUnit::TestCase(name)
 {
 }
 
@@ -54,6 +115,7 @@ void DirectoryIteratorsTest::testDirectoryIterator()
 void DirectoryIteratorsTest::testSortedDirectoryIterator()
 {
 	Path p = path();
+
 	SortedDirectoryIterator dirIterator(p);
 	SortedDirectoryIterator end;
 	std::vector<std::string> result;
@@ -96,6 +158,43 @@ void DirectoryIteratorsTest::testSimpleRecursiveDirectoryIterator()
 }
 
 
+void DirectoryIteratorsTest::testSimpleRecursiveDirectoryIteratorOnError()
+{
+	Path p = path();
+	SimpleRecursiveDirectoryIterator dirIterator(p);
+	dirIterator.onError(*this, &DirectoryIteratorsTest::onError);
+	SimpleRecursiveDirectoryIterator end;
+	std::vector<std::string> result;
+	std::string file;
+
+	Path second(p);
+	second.pushDirectory("first");
+	second.pushDirectory("second");
+	std::string errorPath(second.toString());
+	TemporarilyHidePath hidePath(errorPath);
+
+	while (dirIterator != end)
+	{
+		file = dirIterator->path();
+		++dirIterator;
+		result.push_back(file);
+	}
+
+#if defined(POCO_OS_FAMILY_UNIX)
+	assert(_onErrorPath.size() > 0);
+	if (second.separator() != *_onErrorPath.rbegin())
+		_onErrorPath += second.separator();
+	if (second.separator() != *errorPath.rbegin())
+		errorPath += second.separator();
+
+	assertEquals(_onErrorPath, errorPath);
+	assertEquals(14, (long) result.size());
+#else
+	assertEquals(20, (long) result.size());
+#endif
+}
+
+
 void DirectoryIteratorsTest::testSiblingsFirstRecursiveDirectoryIterator()
 {
 	Path p = path();
@@ -115,6 +214,42 @@ void DirectoryIteratorsTest::testSiblingsFirstRecursiveDirectoryIterator()
 }
 
 
+void DirectoryIteratorsTest::testSiblingsFirstRecursiveDirectoryIteratorOnError()
+{
+	Path p = path();
+	SiblingsFirstRecursiveDirectoryIterator dirIterator(p);
+	dirIterator.onError(*this, &DirectoryIteratorsTest::onError);
+	SimpleRecursiveDirectoryIterator end;
+	std::vector<std::string> result;
+	std::string file;
+
+	Path first(p);
+	first.pushDirectory("first");
+	std::string errorPath(first.toString());
+	TemporarilyHidePath hidePath(errorPath);
+
+	while (dirIterator != end)
+	{
+		file = dirIterator->path();
+		++dirIterator;
+		result.push_back(file);
+	}
+
+#if defined(POCO_OS_FAMILY_UNIX)
+	assert(_onErrorPath.size() > 0);
+	if (first.separator() != *_onErrorPath.rbegin())
+		_onErrorPath += first.separator();
+	if (first.separator() != *errorPath.rbegin())
+		errorPath += first.separator();
+
+	assertEquals(_onErrorPath, errorPath);
+	assertEquals(7, (long) result.size());
+#else
+	assertEquals(20, (long) result.size());
+#endif
+}
+
+
 void DirectoryIteratorsTest::setUp()
 {
 	File d(path());
@@ -171,6 +306,12 @@ void DirectoryIteratorsTest::createSubdir(Path& p)
 }
 
 
+void DirectoryIteratorsTest::onError(const void* pSender, const std::string& path)
+{
+	_onErrorPath = path;
+}
+
+
 void DirectoryIteratorsTest::tearDown()
 {
 	try
@@ -199,7 +340,9 @@ CppUnit::Test* DirectoryIteratorsTest::suite()
 	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testDirectoryIterator);
 	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testSortedDirectoryIterator);
 	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testSimpleRecursiveDirectoryIterator);
+	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testSimpleRecursiveDirectoryIteratorOnError);
 	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testSiblingsFirstRecursiveDirectoryIterator);
+	CppUnit_addTest(pSuite, DirectoryIteratorsTest, testSiblingsFirstRecursiveDirectoryIteratorOnError);
 
 	return pSuite;
 }

Foundation/testsuite/src/DirectoryIteratorsTest.h

@@ -28,8 +28,9 @@ public:
 	void testDirectoryIterator();
 	void testSortedDirectoryIterator();
 	void testSimpleRecursiveDirectoryIterator();
+	void testSimpleRecursiveDirectoryIteratorOnError();
 	void testSiblingsFirstRecursiveDirectoryIterator();
-	
+	void testSiblingsFirstRecursiveDirectoryIteratorOnError();
 	void setUp();
 	void tearDown();
 
@@ -38,6 +39,8 @@ public:
 protected:
 	Poco::Path path() const;
 	void createSubdir(Poco::Path& p);
+	void onError(const void* pSender, const std::string& path);
+	std::string _onErrorPath;
 
 private:
 };

Foundation/testsuite/src/FIFOEventTest.h

@@ -63,7 +63,7 @@ protected:
 
 	int getCount() const;
 private:
-	int		_count;
+	int _count;
 };