Merge pull request #1187 from asarium/fix/1182_2

Show error message when a data directory uses the wrong case

Author: The-E

code/cfile/cfilesystem.cpp

@@ -15,12 +15,25 @@
 #include <errno.h>
 #include <sstream>
 #include <algorithm>
+#include <memory>
 
 #ifdef _WIN32
 #include <io.h>
 #include <direct.h>
 #include <windows.h>
 #include <winbase.h>		/* needed for memory mapping of file functions */
+#include <shlwapi.h>
+
+struct dir_handle_deleter {
+	typedef HANDLE pointer;
+
+	void operator()(HANDLE dirp) {
+		if (dirp != INVALID_HANDLE_VALUE) {
+			FindClose(dirp);
+		}
+	}
+};
+typedef std::unique_ptr<HANDLE, dir_handle_deleter> unique_dir_handle_ptr;
 #endif
 
 #ifdef SCP_UNIX
@@ -30,6 +43,14 @@
 #include <fnmatch.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include <libgen.h>
+
+struct dir_deleter {
+	void operator()(DIR* dirp) {
+		closedir(dirp);
+	}
+};
+typedef std::unique_ptr<DIR, dir_deleter> unique_dir_ptr;
 #endif
 
 #include "cfile/cfile.h"
@@ -38,6 +59,7 @@
 #include "globalincs/pstypes.h"
 #include "localization/localize.h"
 #include "osapi/osapi.h"
+#include "parse/parselo.h"
 
 #define CF_ROOTTYPE_PATH 0
 #define CF_ROOTTYPE_PACK 1
@@ -552,6 +574,62 @@ void cf_search_root_path(int root_index)
 		} 
 
 #if defined _WIN32
+		{
+			// Check if the case matches the case as specified in Pathtypes
+			// Since Windows paths are case insensitive this wouldn't cause issues here but other
+			// platforms would fail to find data paths in this case so we show a nice error if we detect that here
+
+			// Ignore the root since the case of that is allowed to differ (it's handled in the mod handling)
+			if (i != CF_TYPE_ROOT) {
+				// We use FindFirstFileNameW for this, it should hopefully work...
+				// First, convert our path from ASCII/UTF-8 to wchar_t
+				std::string search_string = search_path;
+				// Remove any trailing directory separators
+				if (search_string[search_string.size() - 1] == '\\') {
+					search_string = search_string.substr(0, search_string.size() - 1);
+				}
+
+				char parent_name[MAX_PATH];
+				memset(parent_name, 0, sizeof(parent_name));
+				strcpy_s(parent_name, search_string.c_str());
+
+				CHAR file_name[MAX_PATH];
+				memset(file_name, 0, sizeof(file_name));
+				strcpy_s(file_name, search_string.c_str());
+
+				PathStripPathA(file_name);
+				if (PathRemoveFileSpecA(parent_name)) {
+					strcat_s(parent_name, "\\*");
+
+					WIN32_FIND_DATAA find_data;
+					auto handle = unique_dir_handle_ptr(FindFirstFileA(parent_name, &find_data));
+					if (handle.get() != INVALID_HANDLE_VALUE) {
+						do {
+							if (stricmp(find_data.cFileName, file_name)) {
+								// Not the same name, not even if we check case-insensitive
+								continue;
+							}
+
+							// Same name, might have case differences
+							if (!strcmp(find_data.cFileName, file_name)) {
+								// Case matches, everything is alright.
+								continue;
+							}
+
+							// We need to do some formatting on the parent_name in order to show a nice error message
+							SCP_string parent_name_str = parent_name;
+							parent_name_str = parent_name_str.substr(0, parent_name_str.size() - 1); // Remove trailing *
+							parent_name_str += find_data.cFileName;
+
+							// If we are still here then the case didn't match which means that we have to show the error message
+							Error(LOCATION, "Data directory '%s' for path type '%s' does not match the required case! "
+								"All data directories must exactly match the case specified by the engine or your mod "
+								"will not work on other platforms.", parent_name_str.c_str(), Pathtypes[i].path);
+						} while (FindNextFileA(handle.get(), &find_data) != 0);
+					}
+				}
+			}
+		}
 		strcat_s( search_path, "*.*" );
 
 		intptr_t find_handle;
@@ -588,21 +666,65 @@ void cf_search_root_path(int root_index)
 			_findclose( find_handle );
 		}
 #elif defined SCP_UNIX
-		DIR *dirp;
-		struct dirent *dir;
+		auto dirp = unique_dir_ptr(opendir(search_path));
+		if (!dirp)
+		{
+			// If the directory does not exist then check if it might exist with a different case. If that's the case
+			// we bail out with an error so inform the user that this is not valid.
+
+			// On Unix we can have a different case for the search paths so we also need to account for that
+			// We do that by looking at the parent of search_path and enumerating all directories and then check if any of
+			// them are a case-insensitive match
+			char dirname_copy[CF_MAX_PATHNAME_LENGTH];
+			memcpy(dirname_copy, search_path, sizeof(search_path));
+			// According to the documentation of directory_name and basename, the return value does not need to be freed
+			auto directory_name = dirname(dirname_copy);
+
+			char basename_copy[CF_MAX_PATHNAME_LENGTH];
+			memcpy(basename_copy, search_path, sizeof(search_path));
+			// According to the documentation of dirname and basename, the return value does not need to be freed
+			auto search_name = basename(basename_copy);
+
+			auto parentDirP = unique_dir_ptr(opendir(directory_name));
+			if (parentDirP) {
+				struct dirent *dir = nullptr;
+				while ((dir = readdir (parentDirP.get())) != nullptr) {
+
+					if (stricmp(search_name, dir->d_name)) {
+						continue;
+					}
+
+					SCP_string fn;
+					sprintf(fn, "%s/%s", directory_name, dir->d_name);
+
+					struct stat buf;
+					if (stat(fn.c_str(), &buf) == -1) {
+						continue;
+					}
 
-		dirp = opendir (search_path);
-		if ( dirp ) {
-			while ((dir = readdir (dirp)) != NULL)
+					if (S_ISDIR(buf.st_mode)) {
+						// Found a case insensitive match
+						// If the name is not exactly the same as the one we are currently searching then it's an error
+						if (strcmp(search_name, dir->d_name)) {
+							Error(LOCATION, "Data directory '%s' for path type '%s' does not match the required case! "
+								"All data directories must exactly match the case specified by the engine or your mod "
+								"will not work on other platforms.", fn.c_str(), Pathtypes[i].path);
+						}
+						break;
+					}
+				}
+			}
+		} else {
+			struct dirent *dir = nullptr;
+			while ((dir = readdir (dirp.get())) != NULL)
 			{
 				if (!fnmatch ("*.*", dir->d_name, 0))
 				{
-					char fn[MAX_PATH];
-					snprintf(fn, MAX_PATH-1, "%s%s", search_path, dir->d_name);
-					fn[MAX_PATH-1] = 0;
+					SCP_string fn;
+					sprintf(fn, "%s%s", search_path, dir->d_name);
 
 					struct stat buf;
-					if (stat(fn, &buf) == -1) {
+					if (stat(fn.c_str(), &buf) == -1) {
 						continue;
 					}
 
@@ -632,7 +754,6 @@ void cf_search_root_path(int root_index)
 					}
 				}
 			}
-			closedir(dirp);
 		}
 #endif
 	}

test/src/cfile/cfile.cpp

@@ -0,0 +1,31 @@
+
+#include <gtest/gtest.h>
+#include <graphics/font.h>
+
+#include "util/FSTestFixture.h"
+
+class CFileTest : public test::FSTestFixture {
+ public:
+	CFileTest() : test::FSTestFixture(INIT_NONE) {
+		pushModDir("cfile");
+	}
+
+ protected:
+	virtual void SetUp() override {
+		test::FSTestFixture::SetUp();
+	}
+	virtual void TearDown() override {
+		test::FSTestFixture::TearDown();
+
+		cfile_close();
+	}
+};
+
+
+TEST_F(CFileTest, wrong_data_case) {
+	SCP_string cfile_dir(TEST_DATA_PATH);
+	cfile_dir += DIR_SEPARATOR_CHAR;
+	cfile_dir += "test"; // Cfile expects something after the path
+
+	ASSERT_THROW(cfile_init(cfile_dir.c_str()), os::dialogs::ErrorException);
+}

test/src/menuui/test_intel_parse.cpp

@@ -7,7 +7,7 @@
 
 class IntelParseTest : public test::FSTestFixture {
  public:
-	IntelParseTest() : test::FSTestFixture(0) {
+	IntelParseTest() : test::FSTestFixture(INIT_CFILE) {
 		pushModDir("menuui");
 		pushModDir("intel_parse");
 	}

test/src/parse/test_parselo.cpp

@@ -7,7 +7,7 @@
 
 class ParseloTest : public test::FSTestFixture {
  public:
-	ParseloTest() : test::FSTestFixture(0) {
+	ParseloTest() : test::FSTestFixture(INIT_CFILE) {
 		pushModDir("parselo");
 	}
 
@@ -162,4 +162,4 @@ TEST(ParseloUtilTest, drop_whitespace) {
 	test_str = "       ";
 	drop_white_space(test_str);
 	ASSERT_EQ(test_str, "");
-}
+}

test/src/scripting/ScriptingTestFixture.cpp

@@ -11,7 +11,7 @@ extern "C" {
 namespace test {
 namespace scripting {
 
-ScriptingTestFixture::ScriptingTestFixture(uint64_t init_flags) : FSTestFixture(init_flags) {
+ScriptingTestFixture::ScriptingTestFixture(uint64_t init_flags) : FSTestFixture(init_flags | INIT_CFILE) {
 	pushModDir("scripting");
 }
 void ScriptingTestFixture::SetUp() {

test/src/scripting/api/base.cpp

@@ -3,7 +3,7 @@
 
 class ScriptingBaseTest : public test::scripting::ScriptingTestFixture {
  public:
-	ScriptingBaseTest() : test::scripting::ScriptingTestFixture(0) {
+	ScriptingBaseTest() : test::scripting::ScriptingTestFixture(INIT_NONE) {
 		pushModDir("base");
 	}
 };

test/src/scripting/api/bitops.cpp

@@ -3,7 +3,7 @@
 
 class BitOpsTest : public test::scripting::ScriptingTestFixture {
  public:
-	BitOpsTest() : test::scripting::ScriptingTestFixture(0) {
+	BitOpsTest() : test::scripting::ScriptingTestFixture(INIT_NONE) {
 		pushModDir("bitops");
 	}
 };
@@ -30,4 +30,4 @@ TEST_F(BitOpsTest, checkBit) {
 
 TEST_F(BitOpsTest, addBit) {
 	this->EvalTestScript();
-}
+}

test/src/scripting/api/enums.cpp

@@ -3,7 +3,7 @@
 
 class EnumsTest : public test::scripting::ScriptingTestFixture {
  public:
-	EnumsTest() : test::scripting::ScriptingTestFixture(0) {
+	EnumsTest() : test::scripting::ScriptingTestFixture(INIT_NONE) {
 		pushModDir("enums");
 	}
 };

test/src/source_groups.cmake

@@ -12,6 +12,10 @@ add_file_folder(root ""
     test_stubs.cpp
 )
 
+add_file_folder(cfile "cfile"
+    cfile/cfile.cpp
+)
+
 add_file_folder(graphics "Globalincs"
     globalincs/test_flagset.cpp
     globalincs/test_safe_strings.cpp

test/src/util/FSTestFixture.cpp

@@ -30,6 +30,11 @@ void test::FSTestFixture::SetUp() {
 
 	os_init("Test", "Test");
 
+	if (!(_initFlags & INIT_CFILE)) {
+		return;
+	}
+
+	// The rest depends on cfile initialization
 	SCP_string cfile_dir(TEST_DATA_PATH);
 	cfile_dir += DIR_SEPARATOR_CHAR;
 	cfile_dir += "test"; // Cfile expects something after the path
@@ -52,19 +57,21 @@ void test::FSTestFixture::SetUp() {
 	}
 }
 void test::FSTestFixture::TearDown() {
-	if (_initFlags & INIT_SHIPS) {
-		ship_close();
-	}
+	if (_initFlags & INIT_CFILE) {
+		if (_initFlags & INIT_SHIPS) {
+			ship_close();
+		}
 
-	if (_initFlags & INIT_GRAPHICS) {
-		io::mouse::CursorManager::shutdown();
+		if (_initFlags & INIT_GRAPHICS) {
+			io::mouse::CursorManager::shutdown();
 
-		bm_unload_all();
+			bm_unload_all();
 
-		gr_close();
-	}
+			gr_close();
+		}
 
-	cfile_close();
+		cfile_close();
+	}
 
 	timer_close();