Improve the loading process of the DbgEng DLLs. Fix Vector35#538 and V…

…ector35#344

core/adapters/dbgengadapter.cpp

@@ -73,9 +73,17 @@ std::string DbgEngAdapter::GetDbgEngPath(const std::string& arch)
 	else
 		path = Settings::Instance()->Get<string>("debugger.x86dbgEngPath");
 
-	if (IsValidDbgEngPaths(path))
-		return path;
+	if (!path.empty())
+	{
+		// If the user has specified the path in the setting, then check it for validity. If it is valid, then use it;
+		// if it is invalid, fail the operation -- do not fallback to the default one
+		if (IsValidDbgEngPaths(path))
+			return path;
+		else
+			return "";
+	}
 
+	// If the user does not specify a path (the default case), find the one from the %APPDATA% or %PROGRAMDATA%
 	char appData[MAX_PATH];
 	// For a system installation, the path is %APPDATA%\Binary Ninja\dbgeng\Windows Kits\10\Debuggers
 	if (SUCCEEDED(SHGetFolderPathA(NULL, CSIDL_APPDATA, NULL, 0, appData)))
@@ -97,54 +105,99 @@ std::string DbgEngAdapter::GetDbgEngPath(const std::string& arch)
 	return "";
 }
 
-bool DbgEngAdapter::LoadDngEngLibraries()
+
+static bool LoadOneDLL(const string& path, const string& name, bool strictCheckPath = true, bool forceUnload = true)
 {
-	auto enginePath = GetDbgEngPath("x64");
-	if (!enginePath.empty())
+	auto handle = GetModuleHandleA(name.c_str());
+	if (handle)
 	{
-		LogDebug("DbgEng libraries in path %s", enginePath.c_str());
-		if (!SetDllDirectoryA(enginePath.c_str()))
-			LogWarn(
-				"Failed to set DLL directory to %s. The debugger is going to load the system dbgeng DLLs, which may "
-				"not work as expected",
-				enginePath.c_str());
-	}
-	else
-	{
-		LogWarn(
-			"debugger.x64dbgEngPath is empty or invalid. The debugger is going to load the system dbgeng DLLs, which "
-		    "may "
-			"not work as expected");
+		LogDebug("Module %s is already loaded before the debugger tries to load it, this is suspicious", name.c_str());
+		if (!strictCheckPath)
+			// The module is already loaded and we do not wish to validate its path, treat it as a success
+			return true;
+
+		char actualPath[MAX_PATH];
+		if (!GetModuleFileNameA(handle, actualPath, MAX_PATH))
+		{
+			LogWarn("Failed to get the path of the loaded %s, error: %lu", name.c_str(), GetLastError());
+			return false;
+		}
+		string path1 = actualPath;
+		std::transform(path1.begin(), path1.end(), path1.begin(), ::toupper);
+		string path2 = path + '\\' + name;
+		std::transform(path2.begin(), path2.end(), path2.begin(), ::toupper);
+		if (path1 == path2)
+			// two paths match, ok
+			return true;
+
+		LogWarn("%s is loaded from %s, but we expect it from %s", name.c_str(), actualPath, path.c_str());
+		if (!forceUnload)
+			return false;
+
+		size_t unloadMaxTries = 100;
+		bool unloaded = false;
+		for (size_t i = 0; i < unloadMaxTries; i++)
+		{
+			FreeLibrary(handle);
+			handle = GetModuleHandleA(name.c_str());
+			if (handle == NULL)
+			{
+				unloaded = true;
+				break;
+			}
+		}
+		if (!unloaded)
+		{
+			LogDebug("Failed to unload module %s", name.c_str());
+			return false;
+		}
+		else
+		{
+			LogDebug("Module %s has been unloaded", name.c_str());
+		}
 	}
 
-	HMODULE handle;
-	handle = LoadLibraryA("dbgcore.dll");
+	auto dllFullPath = path + '\\' + name;
+	handle = LoadLibraryA(dllFullPath.c_str());
 	if (handle == nullptr)
 	{
-		LogWarn("Failed to load dbgcore.dll, %d", GetLastError());
+		LogWarn("Failed to load %s, error: %lu", dllFullPath.c_str(), GetLastError());
 		return false;
 	}
 
-	handle = LoadLibraryA("dbghelp.dll");
-	if (handle == nullptr)
+	return true;
+}
+
+
+bool DbgEngAdapter::LoadDngEngLibraries()
+{
+	auto enginePath = GetDbgEngPath("x64");
+	if (enginePath.empty())
 	{
-		LogWarn("Failed to load dbghelp.dll, %d", GetLastError());
+		LogWarn("The debugger cannot find the path for the DbgEng DLLs. "
+			"If you have set debugger.x64dbgEngPath, check its correctness; if you have not set it, "
+			"reinstall the DbgEng redistributable");
 		return false;
 	}
+	LogDebug("DbgEng libraries in path %s", enginePath.c_str());
 
-	handle = LoadLibraryA("dbgmodel.dll");
-	if (handle == nullptr)
-	{
-		LogWarn("Failed to load dbgmodel.dll, %d", GetLastError());
+	auto settings = Settings::Instance();
+	auto strictCheckPath = settings->Get<bool>("debugger.checkDbgEngDLLPath");
+	auto forceUnload = settings->Get<bool>("debugger.tryUnloadWrongDbgEngDLL");
+
+	if (!LoadOneDLL(enginePath, "dbghelp.dll", strictCheckPath, forceUnload))
 		return false;
-	}
 
-	handle = LoadLibraryA("dbgeng.dll");
-	if (handle == nullptr)
-	{
-		LogWarn("Failed to load dbgeng.dll, %d", GetLastError());
+	if (!LoadOneDLL(enginePath, "dbgcore.dll", strictCheckPath, forceUnload))
 		return false;
-	}
+
+	if (!LoadOneDLL(enginePath, "dbgmodel.dll", strictCheckPath, forceUnload))
+		return false;
+
+	if (!LoadOneDLL(enginePath, "dbgeng.dll", strictCheckPath, forceUnload))
+		return false;
+
+	return true;
 }
 
 std::string DbgEngAdapter::GenerateRandomPipeName()
@@ -237,7 +290,11 @@ bool DbgEngAdapter::Start()
 		auto pipeName = GenerateRandomPipeName();
 		auto connectString = fmt::format("npipe:pipe={},Server=localhost", pipeName);
 		auto arch = m_defaultArchitecture == "x86_64" ? "x64" : "x86";
-		auto dbgsrvCommandLine = fmt::format("\"{}\\dbgsrv.exe\" -t {}", GetDbgEngPath(arch), connectString);
+		auto enginePath = GetDbgEngPath(arch);
+		if (enginePath.empty())
+			return false;
+
+		auto dbgsrvCommandLine = fmt::format("\"{}\\dbgsrv.exe\" -t {}", enginePath, connectString);
 		if (!LaunchDbgSrv(dbgsrvCommandLine))
 		{
 			LogWarn("Command %s failed", dbgsrvCommandLine.c_str());
@@ -324,7 +381,7 @@ DbgEngAdapter::~DbgEngAdapter()
 
 bool DbgEngAdapter::Init()
 {
-	return LoadDngEngLibraries();
+	return true;
 }
 
 

core/adapters/dbgengadapter.h

@@ -232,9 +232,9 @@ namespace BinaryNinjaDebugger {
 
 		void ApplyBreakpoints();
 
-		virtual std::string GetDbgEngPath(const std::string& arch);
+		static std::string GetDbgEngPath(const std::string& arch);
 
-		bool LoadDngEngLibraries();
+		static bool LoadDngEngLibraries();
 
 		std::string GenerateRandomPipeName();
 

core/adapters/dbgengttdadapter.cpp

@@ -11,50 +11,6 @@ DbgEngTTDAdapter::DbgEngTTDAdapter(BinaryView* data) : DbgEngAdapter(data)
 }
 
 
-static bool IsValidDbgEngPaths(const std::string& path)
-{
-	if (path.empty())
-		return false;
-
-	auto enginePath = filesystem::path(path);
-	if (!filesystem::exists(enginePath))
-		return false;
-
-	if (!filesystem::exists(enginePath / "dbgeng.dll"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "dbghelp.dll"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "dbgmodel.dll"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "dbgcore.dll"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "dbgsrv.exe"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "ttd"))
-		return false;
-
-	if (!filesystem::exists(enginePath / "ttd" / "TTDReplay.dll"))
-		return false;
-
-	return true;
-}
-
-
-std::string DbgEngTTDAdapter::GetDbgEngPath(const std::string& arch)
-{
-	std::string path = Settings::Instance()->Get<string>("debugger.x64dbgEngPath");
-	if (IsValidDbgEngPaths(path))
-		return path;
-
-	return "";
-}
-
-
 bool DbgEngTTDAdapter::ExecuteWithArgsInternal(const std::string& path, const std::string& args,
                                             const std::string& workingDir, const LaunchConfigurations& configs) {
     m_aboutToBeKilled = false;

core/adapters/dbgengttdadapter.h

@@ -32,8 +32,6 @@ namespace BinaryNinjaDebugger {
 		void Reset() override;
 
 		bool Quit() override;
-
-		std::string GetDbgEngPath(const std::string& arch) override;
     };
 
     class DbgEngTTDAdapterType : public DebugAdapterType

core/adapters/lldbadapter.cpp

@@ -74,14 +74,16 @@ DebugAdapter* LldbAdapterType::Create(BinaryNinja::BinaryView* data)
 	// the liblldb.dll is located.
 	// As a note, the reason for us to apply delay load on liblldb.dll is that if we load it early, it will also load
 	// the system's default dbgeng dlls, which does not work for our dbgeng adapter.
+	std::string lldbDir;
 	if (getenv("BN_STANDALONE_DEBUGGER") != nullptr)
-		SetDllDirectoryA(GetUserPluginDirectory().c_str());
+		lldbDir = GetUserPluginDirectory();
 	else
-		SetDllDirectoryA(GetBundledPluginDirectory().c_str());
+		lldbDir = GetBundledPluginDirectory();
 
-	auto module = LoadLibraryA("liblldb.dll");
+	auto lldbPath = lldbDir + '\\' + "liblldb.dll";
+	auto module = LoadLibraryA(lldbPath.c_str());
 	if (module == NULL)
-		throw std::runtime_error("fail to load liblldb");
+		throw std::runtime_error(std::string("fail to load ") + lldbPath);
 #endif
 
 	// TODO: someone should free this.

core/adapters/localwindowskerneladapter.cpp

@@ -13,7 +13,6 @@ LocalWindowsKernelAdapter::LocalWindowsKernelAdapter(BinaryView* data) : DbgEngA
 
 bool LocalWindowsKernelAdapter::ExecuteWithArgsInternal(const std::string& path, const std::string& args,
                                             const std::string& workingDir, const LaunchConfigurations& configs) {
-	LogWarn("here");
     m_aboutToBeKilled = false;
 
     if (this->m_debugActive) {

core/adapters/windowskerneladapter.h

@@ -31,8 +31,6 @@ namespace BinaryNinjaDebugger {
 
 		bool Detach() override;
 		bool Quit() override;
-
-//		std::string GetDbgEngPath(const std::string& arch) override;
     };
 
     class WindowsKernelAdapterType : public DebugAdapterType

core/debugger.cpp

@@ -30,6 +30,10 @@ using namespace BinaryNinjaDebugger;
 void InitDebugAdapterTypes()
 {
 #ifdef WIN32
+	if (!DbgEngAdapter::LoadDngEngLibraries())
+	{
+		LogWarn("Failed to load DbgEng DLLs, the DbgEng adapter cannot work properly");
+	}
     InitDbgEngAdapterType();
 	InitDbgEngTTDAdapterType();
 	InitWindowsKernelAdapterType();
@@ -91,6 +95,22 @@ static void RegisterSettings()
 			"description" : "Path of the x86 DbgEng Installation. This folder should contain an x86 dbgeng.dll.",
 			"ignore" : ["SettingsProjectScope", "SettingsResourceScope"]
 			})");
+	settings->RegisterSetting("debugger.checkDbgEngDLLPath",
+		R"({
+			"title" : "Check DbgEng DLL path",
+			"type" : "boolean",
+			"default" : true,
+			"description" : "Check if the DbgEng DLLs are loaded from the correct path. This ensures the debugger is using the correct version of DLLs",
+			"ignore" : ["SettingsProjectScope", "SettingsResourceScope"]
+			})");
+	settings->RegisterSetting("debugger.tryUnloadWrongDbgEngDLL",
+		R"({
+			"title" : "Attempt to unload the DLL with wrong path",
+			"type" : "boolean",
+			"default" : false,
+			"description" : "Attempt to unload the already loaded DLL if they are from a wrong path. You may turn this on if the DbgEng DLLs, e.g., dbghelp.dll, is loaded from a wrong path, but it happens early than the debugger initialization",
+			"ignore" : ["SettingsProjectScope", "SettingsResourceScope"]
+			})");
 #endif
 
 	settings->RegisterSetting("debugger.stackVariableAnnotations",