Replace deprecated readdir_r with readdir

Change-Id: I83e91612be7ff318d0ee932cc6f4b3ea04a79f12
Signed-off-by: Kyungwook Tak <k.tak@samsung.com>

6 files changed, 127 insertions, 42 deletions

diff --git a/src/framework/service/file-system.cpp b/src/framework/service/file-system.cpp
index 19e0aaf..2422a4d 100644
--- a/src/framework/service/file-system.cpp
+++ b/src/framework/service/file-system.cpp
@@ -201,10 +201,51 @@ FilePtr File::createInternal(const std::string &fpath, const FilePtr &parentdir,
 		return FilePtr(new File(fpath, parentdir, type, std::move(statptr)));
 }
 
+std::map<std::string, std::mutex> FsVisitor::m_locks;
+std::mutex FsVisitor::m_lockMapCleanMutex;
+
+void FsVisitor::cleanLocks()
+{
+	std::lock_guard<std::mutex> l(FsVisitor::m_lockMapCleanMutex);
+	FsVisitor::m_locks.clear();
+}
+
+FsVisitor::Dir::Dir(const std::string &dirname) : m_dir(nullptr), m_dirname(dirname)
+{
+	if (isInBlackList(this->m_dirname))
+		throw std::invalid_argument("dirname is in blacklist.");
+
+	DEBUG("try lock() on dir: " << this->m_dirname);
+	// get lock ownership before open directory stream
+	FsVisitor::m_locks[this->m_dirname].lock();
+	DEBUG("get lock for dir: " << this->m_dirname);
+
+	if ((this->m_dir = ::opendir(this->m_dirname.c_str())) == nullptr) {
+		// release lock before throw exception because dtor won't be called.
+		FsVisitor::m_locks[this->m_dirname].unlock();
+		throw std::invalid_argument("cannot open dir");
+	}
+}
+
+FsVisitor::Dir::~Dir()
+{
+	::closedir(this->m_dir);
+	FsVisitor::m_locks[this->m_dirname].unlock();
+	DEBUG("unlocked with closedir: " << this->m_dirname);
+}
+
+DIR *FsVisitor::Dir::get()
+{
+	return this->m_dir;
+}
+
 FsVisitor::DirPtr FsVisitor::openDir(const std::string &dir)
 {
-	return std::unique_ptr<DIR, int(*)(DIR *)>(
-		(isInBlackList(dir) ? nullptr : ::opendir(dir.c_str())), ::closedir);
+	try {
+		return DirPtr(new FsVisitor::Dir(dir));
+	} catch (...) {
+		return DirPtr(nullptr);
+	}
 }
 
 FsVisitorPtr FsVisitor::create(TargetHandler &&targetHandler,
@@ -225,30 +266,28 @@ FsVisitorPtr FsVisitor::create(TargetHandler &&targetHandler,
 FsVisitor::FsVisitor(TargetHandler &&targetHandler, const std::string &dirpath,
 					 bool isBasedOnName, time_t modifiedSince) :
 	m_targetHandler(std::move(targetHandler)), m_path(dirpath),
-	m_since(modifiedSince), m_isDone(true), m_isBasedOnName(isBasedOnName),
-	m_entryBuf(static_cast<struct dirent *>(::malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1)))
+	m_since(modifiedSince), m_isDone(true), m_isBasedOnName(isBasedOnName)
 {
-	if (this->m_entryBuf == nullptr)
-		throw std::bad_alloc();
 }
 
 FsVisitor::~FsVisitor()
 {
-	::free(this->m_entryBuf);
 }
 
 void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 {
-	struct dirent *result = nullptr;
 	while (true) {
-		auto ret = ::readdir_r(dirptr.get(), this->m_entryBuf, &result);
-		if (ret != 0) {
-			WARN("readdir_r error on dir: " << currentdir->getPath() <<
-				 " with errno: " << errno << ". Silently ignore this error & dir stream"
-				 " to reduce side-effect of traversing all the other file systems.");
-			break;
-		} else if (result == nullptr) {
-			DEBUG("End of stream of dir: " << currentdir->getPath());
+		errno = 0;
+		auto result = ::readdir(dirptr->get());
+		if (result == nullptr) {
+			if (errno != 0)
+				WARN("readdir error on dir: " << currentdir->getPath() <<
+					 " with errno: " << errno << ". Silently ignore this error &"
+					 " dir stream to reduce side-effect of traversing all the other"
+					 " file systems.");
+			else
+				DEBUG("end of stream: " << currentdir->getPath());
+
 			break;
 		}
 
@@ -267,12 +306,6 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 				(name_size == 2 && name[0] == '.' && name[1] == '.'))
 				continue;
 
-			auto ndirptr = openDir(fullpath);
-			if (ndirptr == nullptr) {
-				WARN("Failed to open dir: " << fullpath << " with errno: " << errno);
-				continue;
-			}
-
 			FilePtr ncurrentdir;
 			try {
 				ncurrentdir = File::create(fullpath, currentdir);
@@ -288,6 +321,12 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 			if (this->m_isBasedOnName && ncurrentdir->isInApp()) {
 				this->m_targetHandler(ncurrentdir);
 			} else {
+				auto ndirptr = openDir(fullpath);
+				if (ndirptr == nullptr) {
+					WARN("Failed to open dir: " << fullpath << " with errno: " << errno);
+					continue;
+				}
+
 				DEBUG("recurse dir : " << fullpath);
 				this->run(ndirptr, ncurrentdir);
 			}
@@ -312,6 +351,15 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 
 void FsVisitor::run()
 {
+	auto currentdir = File::create(this->m_path, nullptr);
+
+	INFO("Visiting files start from dir: " << this->m_path);
+
+	if (this->m_isBasedOnName && currentdir->isInApp()) {
+		this->m_targetHandler(currentdir);
+		return;
+	}
+
 	auto dirptr = openDir(this->m_path);
 
 	if (dirptr == nullptr) {
@@ -327,14 +375,7 @@ void FsVisitor::run()
 		}
 	}
 
-	auto currentdir = File::create(this->m_path, nullptr);
-
-	INFO("Visiting files start from dir: " << this->m_path);
-
-	if (this->m_isBasedOnName && currentdir->isInApp())
-		this->m_targetHandler(currentdir);
-	else
-		this->run(dirptr, currentdir);
+	this->run(dirptr, currentdir);
 }
 
 } // namespace Csr
diff --git a/src/framework/service/file-system.h b/src/framework/service/file-system.h
index 70f6ca8..ee73279 100644
--- a/src/framework/service/file-system.h
+++ b/src/framework/service/file-system.h
@@ -25,6 +25,8 @@
 #include <memory>
 #include <queue>
 #include <functional>
+#include <mutex>
+#include <map>
 
 #include <cstddef>
 #include <ctime>
@@ -147,8 +149,25 @@ public:
 							   const std::string &dirpath, bool isBasedOnName,
 							   time_t modifiedSince = -1);
 
+	// clean up per directory lock map.
+	// Warning: should be called only no directory is traversed at the time.
+	// this is essential because mutex exists per directory and it can be
+	// stacked a lot because it won't be freed unless process is terminated.
+	static void cleanLocks();
+
 private:
-	using DirPtr = std::unique_ptr<DIR, int(*)(DIR *)>;
+	class Dir {
+	public:
+		Dir(const std::string &dirname);
+		~Dir();
+		DIR *get();
+
+	private:
+		DIR *m_dir;
+		std::string m_dirname;
+	};
+
+	using DirPtr = std::unique_ptr<Dir>;
 
 	void run(const DirPtr &dirptr, const FilePtr &currentdir);
 
@@ -157,12 +176,14 @@ private:
 	FsVisitor(TargetHandler &&targetHandler, const std::string &dirpath,
 			  bool isBasedOnName, time_t modifiedSince = -1);
 
+	static std::map<std::string, std::mutex> m_locks;
+	static std::mutex m_lockMapCleanMutex;
+
 	TargetHandler m_targetHandler;
 	std::string m_path;
 	time_t m_since;
 	bool m_isDone;
 	bool m_isBasedOnName;
-	struct dirent *m_entryBuf;
 };
 
 } // namespace Csr
diff --git a/src/framework/service/thread-pool.cpp b/src/framework/service/thread-pool.cpp
index c28ac45..218c8ff 100644
--- a/src/framework/service/thread-pool.cpp
+++ b/src/framework/service/thread-pool.cpp
@@ -28,6 +28,10 @@
 #include "common/audit/logger.h"
 #include "common/exception.h"
 
+#ifndef INTERNAL_TEST_BUILD
+#include "service/file-system.h"
+#endif
+
 #define __BEGIN_CRITICAL__ { std::unique_lock<std::mutex> lock(this->m_mutex);
 #define __END_CRITICAL__   }
 
@@ -59,11 +63,15 @@ ThreadPool::ThreadPool(size_t threads) : m_stop(false), m_runningWorkersNum(0)
 
 				DEBUG("Start task on thread[" << std::this_thread::get_id() << "]");
 
+				__BEGIN_CRITICAL__
 				++this->m_runningWorkersNum;
+				__END_CRITICAL__
 
 				task();
 
+				__BEGIN_CRITICAL__
 				--this->m_runningWorkersNum;
+				__END_CRITICAL__
 			}
 		});
 	}
@@ -73,7 +81,16 @@ ThreadPool::ThreadPool(size_t threads) : m_stop(false), m_runningWorkersNum(0)
 
 bool ThreadPool::isTaskRunning() const
 {
-	return this->m_runningWorkersNum != 0;
+	std::lock_guard<std::mutex> l(this->m_mutex);
+
+	if (this->m_runningWorkersNum == 0) {
+#ifndef INTERNAL_TEST_BUILD
+		FsVisitor::cleanLocks();
+#endif
+		return false;
+	} else {
+		return true;
+	}
 }
 
 ThreadPool::~ThreadPool()
diff --git a/src/framework/service/thread-pool.h b/src/framework/service/thread-pool.h
index a808ffa..2f9ac80 100644
--- a/src/framework/service/thread-pool.h
+++ b/src/framework/service/thread-pool.h
@@ -24,7 +24,6 @@
 
 #include <thread>
 #include <mutex>
-#include <atomic>
 #include <condition_variable>
 #include <functional>
 #include <vector>
@@ -51,10 +50,10 @@ public:
 
 private:
 	bool m_stop;
-	std::atomic<size_t> m_runningWorkersNum;
+	size_t m_runningWorkersNum;
 	std::vector<std::thread> m_workers;
 	std::queue<std::function<void()>> m_tasks;
-	std::mutex m_mutex;
+	mutable std::mutex m_mutex;
 	std::condition_variable m_cv;
 };
 
diff --git a/test/engine/content-screening/sample-engine.cpp b/test/engine/content-screening/sample-engine.cpp
index 11063ca..34afff7 100644
--- a/test/engine/content-screening/sample-engine.cpp
+++ b/test/engine/content-screening/sample-engine.cpp
@@ -465,20 +465,21 @@ int csre_cs_scan_app_on_cloud(csre_cs_context_h handle,
 		return CSRE_ERROR_NONE;
 	}
 
-	struct dirent entry;
-	struct dirent *result;
+	while (true) {
+		auto result = readdir(dirp.get());
+		if (result == nullptr)
+			break;
 
-	while (readdir_r(dirp.get(), &entry, &result) == 0 && result != nullptr) {
 		std::string fullpath(app_root_dir);
-		std::string filename(entry.d_name);
+		std::string filename(result->d_name);
 		fullpath += "/";
 		fullpath += filename;
 
 		int ret = CSRE_ERROR_NONE;
 
-		if (entry.d_type & (DT_REG | DT_LNK))
+		if (result->d_type & (DT_REG | DT_LNK))
 			ret = csre_cs_scan_file(handle, fullpath.c_str(), &detected);
-		else if ((entry.d_type & DT_DIR)
+		else if ((result->d_type & DT_DIR)
 				 && filename.compare("..") != 0
 				 && filename.compare(".") != 0)
 			ret = csre_cs_scan_app_on_cloud(handle, fullpath.c_str(), &detected);
diff --git a/test/thread-pool/CMakeLists.txt b/test/thread-pool/CMakeLists.txt
index 41a2c58..a569f1e 100644
--- a/test/thread-pool/CMakeLists.txt
+++ b/test/thread-pool/CMakeLists.txt
@@ -16,6 +16,7 @@
 # @author      Kyungwook Tak (k.tak@samsung.com)
 # @brief       build test program of server thread pool
 #
+
 PKG_CHECK_MODULES(${TARGET_CSR_THREADPOOL_TEST}_DEP
 	REQUIRED
 )
@@ -36,6 +37,11 @@ INCLUDE_DIRECTORIES(
 	${${TARGET_CSR_THREADPOOL_TEST}_DEP_INCLUDE_DIRS}
 )
 
+SET_SOURCE_FILES_PROPERTIES(${${TARGET_CSR_THREADPOOL_TEST}_SRCS}
+	PROPERTIES
+		COMPILE_FLAGS "-DINTERNAL_TEST_BUILD"
+)
+
 ADD_EXECUTABLE(${TARGET_CSR_THREADPOOL_TEST} ${${TARGET_CSR_THREADPOOL_TEST}_SRCS})
 
 TARGET_LINK_LIBRARIES(${TARGET_CSR_THREADPOOL_TEST}