From c8f1efebeaca041959859f2547931bcd108a35fc Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Thu, 10 Oct 2024 17:40:06 +0200
Subject: [PATCH] Use execvp in fs::RecursiveDelete()
---
src/filesys.cpp | 73 ++++++++++++++++++-----------------
src/unittest/test_filesys.cpp | 31 +++++++++++++++
2 files changed, 69 insertions(+), 35 deletions(-)
diff --git a/src/filesys.cpp b/src/filesys.cpp
index 196aca080..b0a1f318e 100644
--- a/src/filesys.cpp
+++ b/src/filesys.cpp
@@ -35,6 +35,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#include <IFileArchive.h>
#include <IFileSystem.h>
#endif
+
#ifdef __linux__
#include <fcntl.h>
#include <sys/ioctl.h>
@@ -43,6 +44,19 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#endif
#endif
+#ifdef _WIN32
+#include <windows.h>
+#include <shlwapi.h>
+#include <io.h>
+#include <direct.h>
+#else
+#include <sys/types.h>
+#include <dirent.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#endif
+
// Error from last OS call as string
#ifdef _WIN32
#define LAST_OS_ERROR() porting::ConvertError(GetLastError())
@@ -59,11 +73,6 @@ namespace fs
* Windows *
***********/
-#include <windows.h>
-#include <shlwapi.h>
-#include <io.h>
-#include <direct.h>
-
std::vector<DirListNode> GetDirListing(const std::string &pathstring)
{
std::vector<DirListNode> listing;
@@ -273,12 +282,6 @@ bool CopyFileContents(const std::string &source, const std::string &target)
* POSIX *
*********/
-#include <sys/types.h>
-#include <dirent.h>
-#include <sys/stat.h>
-#include <sys/wait.h>
-#include <unistd.h>
-
std::vector<DirListNode> GetDirListing(const std::string &pathstring)
{
std::vector<DirListNode> listing;
@@ -381,41 +384,41 @@ bool RecursiveDelete(const std::string &path)
Execute the 'rm' command directly, by fork() and execve()
*/
- infostream<<"Removing \""<<path<<"\""<<std::endl;
+ infostream << "Removing \"" << path << "\"" << std::endl;
- pid_t child_pid = fork();
+ assert(IsPathAbsolute(path));
- if(child_pid == 0)
- {
+ const pid_t child_pid = fork();
+
+ if (child_pid == -1) {
+ errorstream << "fork errno: " << errno << ": " << strerror(errno)
+ << std::endl;
+ return false;
+ }
+
+ if (child_pid == 0) {
// Child
- const char *argv[4] = {
-#ifdef __ANDROID__
- "/system/bin/rm",
-#else
- "/bin/rm",
-#endif
+ std::array<const char*, 4> argv = {
+ "rm",
"-rf",
path.c_str(),
- NULL
+ nullptr
};
- verbosestream<<"Executing '"<<argv[0]<<"' '"<<argv[1]<<"' '"
- <<argv[2]<<"'"<<std::endl;
+ execvp(argv[0], const_cast<char**>(argv.data()));
- execv(argv[0], const_cast<char**>(argv));
-
- // Execv shouldn't return. Failed.
+ // note: use cerr because our logging won't flush in forked process
+ std::cerr << "exec errno: " << errno << ": " << strerror(errno)
+ << std::endl;
_exit(1);
- }
- else
- {
+ } else {
// Parent
- int child_status;
+ int status;
pid_t tpid;
- do{
- tpid = wait(&child_status);
- }while(tpid != child_pid);
- return (child_status == 0);
+ do
+ tpid = waitpid(child_pid, &status, 0);
+ while (tpid != child_pid);
+ return WIFEXITED(status) && WEXITSTATUS(status) == 0;
}
}
diff --git a/src/unittest/test_filesys.cpp b/src/unittest/test_filesys.cpp
index fd25d2de9..e24e79374 100644
--- a/src/unittest/test_filesys.cpp
+++ b/src/unittest/test_filesys.cpp
@@ -42,6 +42,7 @@ public:
void testSafeWriteToFile();
void testCopyFileContents();
void testNonExist();
+ void testRecursiveDelete();
};
static TestFileSys g_test_instance;
@@ -56,6 +57,7 @@ void TestFileSys::runTests(IGameDef *gamedef)
TEST(testSafeWriteToFile);
TEST(testCopyFileContents);
TEST(testNonExist);
+ TEST(testRecursiveDelete);
}
////////////////////////////////////////////////////////////////////////////////
@@ -338,3 +340,32 @@ void TestFileSys::testNonExist()
auto ifs = open_ifstream(path.c_str(), false);
UASSERT(!ifs.good());
}
+
+void TestFileSys::testRecursiveDelete()
+{
+ std::string dirs[2];
+ dirs[0] = getTestTempDirectory() + DIR_DELIM "a";
+ dirs[1] = dirs[0] + DIR_DELIM "b";
+
+ std::string files[2] = {
+ dirs[0] + DIR_DELIM "file1",
+ dirs[1] + DIR_DELIM "file2"
+ };
+
+ for (auto &it : dirs)
+ fs::CreateDir(it);
+ for (auto &it : files)
+ open_ofstream(it.c_str(), false).close();
+
+ for (auto &it : dirs)
+ UASSERT(fs::IsDir(it));
+ for (auto &it : files)
+ UASSERT(fs::IsFile(it));
+
+ UASSERT(fs::RecursiveDelete(dirs[0]));
+
+ for (auto &it : dirs)
+ UASSERT(!fs::IsDir(it));
+ for (auto &it : files)
+ UASSERT(!fs::IsFile(it));
+}