From 01d21fd2c091cbf23d967b0971d18c88da64a0b4 Mon Sep 17 00:00:00 2001 From: chenkeyu Date: Mon, 11 Mar 2024 10:42:58 +0800 Subject: [PATCH] Fix potential recursion stack overflow and long path handling in GetDirFiles Issue:https://gitee.com/openharmony/commonlibrary_c_utils/issues/I97CNQ?from=project-issue Signed-off-by: chenkeyu --- base/src/directory_ex.cpp | 45 +++-- .../unittest/common/utils_directory_test.cpp | 162 ++++++++++++++++-- 2 files changed, 182 insertions(+), 25 deletions(-) diff --git a/base/src/directory_ex.cpp b/base/src/directory_ex.cpp index 77dddb7..5947ba3 100644 --- a/base/src/directory_ex.cpp +++ b/base/src/directory_ex.cpp @@ -149,29 +149,52 @@ string IncludeTrailingPathDelimiter(const std::string& path) void GetDirFiles(const string& path, vector& files) { - string pathStringWithDelimiter; DIR *dir = opendir(path.c_str()); if (dir == nullptr) { + UTILS_LOGD("Failed to open root dir: %{public}s: %{public}s ", path.c_str(), strerror(errno)); return; } - while (true) { - struct dirent *ptr = readdir(dir); + string currentPath = ExcludeTrailingPathDelimiter(path); + stack traverseStack; + traverseStack.push(dir); + while (!traverseStack.empty()) { + DIR *topNode = traverseStack.top(); + dirent *ptr = readdir(topNode); if (ptr == nullptr) { - break; + closedir(topNode); + traverseStack.pop(); + currentPath.erase(currentPath.find_last_of("/")); + continue; } - // current dir or parent dir - if ((strcmp(ptr->d_name, ".") == 0) || (strcmp(ptr->d_name, "..") == 0)) { + string name = ptr->d_name; + if (name == "." || name == "..") { continue; - } else if (ptr->d_type == DT_DIR) { - pathStringWithDelimiter = IncludeTrailingPathDelimiter(path) + string(ptr->d_name); - GetDirFiles(pathStringWithDelimiter, files); + } + if (ptr->d_type == DT_DIR) { + int currentFd = dirfd(topNode); + if (currentFd < 0) { + UTILS_LOGD("Failed to get dirfd, fd: %{public}d: %{public}s ", currentFd, strerror(errno)); + continue; + } + int subFd = openat(currentFd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (subFd < 0) { + UTILS_LOGD("Failed in subFd openat: %{public}s ", name.c_str()); + continue; + } + DIR *subDir = fdopendir(subFd); + if (subDir == nullptr) { + close(subFd); + UTILS_LOGD("Failed in fdopendir: %{public}s", strerror(errno)); + continue; + } + traverseStack.push(subDir); + currentPath = IncludeTrailingPathDelimiter(currentPath) + name; } else { - files.push_back(IncludeTrailingPathDelimiter(path) + string(ptr->d_name)); + files.push_back(IncludeTrailingPathDelimiter(currentPath) + name); } } - closedir(dir); } bool ForceCreateDirectory(const string& path) diff --git a/base/test/unittest/common/utils_directory_test.cpp b/base/test/unittest/common/utils_directory_test.cpp index 019d0f1..265571b 100644 --- a/base/test/unittest/common/utils_directory_test.cpp +++ b/base/test/unittest/common/utils_directory_test.cpp @@ -19,6 +19,8 @@ #include #include #include +#include +#include using namespace testing::ext; using namespace std; @@ -139,25 +141,157 @@ HWTEST_F(UtilsDirectoryTest, testIncludeTrailingPathDelimiter001, TestSize.Level /* * @tc.name: testGetDirFiles001 - * @tc.desc: directory unit test + * @tc.desc: test GetDirFiles works on multi-level directory */ HWTEST_F(UtilsDirectoryTest, testGetDirFiles001, TestSize.Level0) { - string resultfile[2] = { "/data/test/TestFile.txt", "/data/test/UtilsDirectoryTest" }; - // prepare test data - ofstream file(resultfile[0], fstream::out); + string parent_path = "/data/test_dir"; + + ForceCreateDirectory(parent_path); + + string dirs[6] = { + "/data/test_dir/level1_1", + "/data/test_dir/level1_2", + "/data/test_dir/level1_2/level2_1", + "/data/test_dir/level1_2/level2_2", + "/data/test_dir/level1_2/level2_2/level3_1", + "/data/test_dir/level1_3", + }; + + string resultfiles[9] = { + "/data/test_dir/level1_1/test_file", + "/data/test_dir/level1_2/level2_2/level3_1/test_file_1", + "/data/test_dir/level1_2/level2_2/level3_1/test_file_2", + "/data/test_dir/level1_2/level2_2/test_file_1", + "/data/test_dir/level1_2/level2_2/test_file_2", + "/data/test_dir/level1_2/level2_2/test_file_3", + "/data/test_dir/level1_2/level2_2/test_file_4", + "/data/test_dir/level1_2/test_file", + "/data/test_dir/level1_3/test_file", + }; + + for (auto &path : dirs) { + ForceCreateDirectory(path); + } + + for (auto &filepath : resultfiles) { + ofstream(filepath, fstream::out); + } + + vector files; + + GetDirFiles(parent_path, files); + + for (auto &filepath : resultfiles) { + auto pos = find(files.begin(), files.end(), filepath); + EXPECT_NE(pos, files.end()); + } + + ForceRemoveDirectory(parent_path); +} + +/* + * @tc.name: testGetDirFiles002 + * @tc.desc: test GetDirFiles works on deeply nested directory and handles very long path + */ +HWTEST_F(UtilsDirectoryTest, testGetDirFiles002, TestSize.Level0) +{ + string parentPath = "/data/test_dir/"; + string veryLongPath = "/data/test_dir/"; + + int length = 10000; + + for (int i = 0; i < length; i++) { + veryLongPath += "0"; + veryLongPath += "/"; + } + + EXPECT_EQ(mkdir("/data/test_dir", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH), 0); + chdir(parentPath.c_str()); + + for (int i = 0; i < length; i++) { + EXPECT_EQ(mkdir("./0", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH), 0); + EXPECT_EQ(chdir("./0"), 0); + } + + ofstream file("./test_file"); + file.close(); + EXPECT_EQ(chdir("/data/test"), 0); + + auto files = vector(); + + GetDirFiles(parentPath, files); + + EXPECT_EQ(files.size(), 1); + EXPECT_EQ((veryLongPath + "test_file").length(), files[0].length()); + EXPECT_EQ(veryLongPath + "test_file", files[0]); + + ForceRemoveDirectory(parentPath); +} + +/* + * @tc.name: testGetDirFiles003 + * @tc.desc: test GetDirFiles works on symlink + */ +HWTEST_F(UtilsDirectoryTest, testGetDirFiles003, TestSize.Level0) +{ + // create a test dir + string original_data_path = "/data/original"; + EXPECT_EQ(ForceCreateDirectory(original_data_path), true); + + string original_file_path = "/data/original/original_file"; + string original_directory_path = "/data/original/original_directory"; + + ofstream(original_file_path, fstream::out); + + ForceCreateDirectory(original_directory_path); + + string test_data_dir = "/data/test_dir"; + + EXPECT_EQ(ForceCreateDirectory(test_data_dir), true); + + // test symlink to directory outside the target directory + string linktodir = IncludeTrailingPathDelimiter(test_data_dir) + "symlink_dir"; + + EXPECT_EQ(symlink(original_directory_path.c_str(), linktodir.c_str()), 0); + + vector dir_result; + GetDirFiles(test_data_dir, dir_result); + + EXPECT_EQ(dir_result.size(), 1); + EXPECT_EQ(dir_result[0], linktodir); + + EXPECT_EQ(ForceRemoveDirectory(linktodir), true); + + // test symlink to file outside the target directory + string linktofile = IncludeTrailingPathDelimiter(test_data_dir) + "symlink_file"; + EXPECT_EQ(symlink(original_file_path.c_str(), linktofile.c_str()), 0); + + vector file_result; + GetDirFiles(test_data_dir, file_result); + EXPECT_EQ(file_result.size(), 1); + EXPECT_EQ(file_result[0], linktofile); + + EXPECT_EQ(RemoveFile(linktofile), true); + + // test symlink of files in the same directory + string source_file = IncludeTrailingPathDelimiter(test_data_dir) + "source"; + string symlink_file = IncludeTrailingPathDelimiter(test_data_dir) + "symlink_file"; + + ofstream(source_file, fstream::out); + EXPECT_EQ(symlink(source_file.c_str(), symlink_file.c_str()), 0); + + vector internal_files; + GetDirFiles(test_data_dir, internal_files); - string dirpath = "/data/"; - vector filenames; - GetDirFiles(dirpath, filenames); - auto pos = find(filenames.begin(), filenames.end(), resultfile[0]); - EXPECT_NE(pos, filenames.end()); + EXPECT_NE(find(internal_files.begin(), internal_files.end(), source_file), internal_files.end()); + EXPECT_NE(find(internal_files.begin(), internal_files.end(), symlink_file), internal_files.end()); - pos = find(filenames.begin(), filenames.end(), resultfile[1]); - EXPECT_NE(pos, filenames.end()); + EXPECT_EQ(RemoveFile(source_file), true); + EXPECT_EQ(RemoveFile(symlink_file), true); - // delete test data - RemoveFile(resultfile[0]); + ForceRemoveDirectory(original_data_path); + ForceRemoveDirectory(test_data_dir); } /* @@ -438,4 +572,4 @@ HWTEST_F(UtilsDirectoryTest, testPathToRealPath006, TestSize.Level0) EXPECT_EQ(ret, false); } } // namespace -} // namespace OHOS \ No newline at end of file +} // namespace OHOS -- Gitee