Merge pull request #480 from sfall-team/feature/debug-log-overflow-fix

Fixed buffer overflow when DebugMode is enabled and printing messages longer than 259 via script.

sfall/ConsoleWindow.cpp

@@ -0,0 +1,145 @@
+/*
+ *    sfall
+ *    Copyright (C) 2008-2023  The sfall team
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the GNU General Public License as published by
+ *    the Free Software Foundation, either version 3 of the License, or
+ *    (at your option) any later version.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    GNU General Public License for more details.
+ *
+ *    You should have received a copy of the GNU General Public License
+ *    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "ConsoleWindow.h"
+
+#include "FalloutEngine\Fallout2.h"
+#include "IniReader.h"
+#include "Logging.h"
+#include "SafeWrite.h"
+#include "Utils.h"
+
+#include <iostream>
+#include <sstream>
+
+namespace sfall
+{
+
+static constexpr char* IniSection = "Debugging";
+static constexpr char* IniModeKey = "ConsoleWindow";
+static constexpr char* IniPositionKey = "ConsoleWindowData";
+static constexpr char* IniCodePageKey = "ConsoleCodePage";
+
+ConsoleWindow ConsoleWindow::_instance;
+
+bool ConsoleWindow::tryGetWindow(HWND* wnd) {
+	*wnd = GetConsoleWindow();
+	if (!*wnd) {
+		dlogr("Error getting console window.", DL_MAIN);
+		return false;
+	}
+	return true;
+}
+
+void ConsoleWindow::loadPosition() {
+	auto windowDataStr = IniReader::GetStringDefaultConfig(IniSection, IniPositionKey, "");
+	auto windowDataSplit = split(windowDataStr, ',');
+	if (windowDataSplit.size() < 4) return;
+
+	HWND wnd;
+	if (!tryGetWindow(&wnd)) return;
+
+	int windowData[4];
+	for (size_t i = 0; i < 4; i++) {
+		windowData[i] = atoi(windowDataSplit.at(i).c_str());
+	}
+	if (!SetWindowPos(wnd, HWND_TOP, windowData[0], windowData[1], windowData[2], windowData[3], 0)) {
+		dlog_f("Error repositioning console window: 0x%x\n", DL_MAIN, GetLastError());
+	}
+}
+
+void ConsoleWindow::savePosition() {
+	HWND wnd;
+	if (!tryGetWindow(&wnd)) return;
+
+	RECT wndRect;
+	if (!GetWindowRect(wnd, &wndRect)) {
+		dlog_f("Error getting console window position: 0x%x\n", DL_MAIN, GetLastError());
+	}
+	int width = wndRect.right - wndRect.left;
+	int height = wndRect.bottom - wndRect.top;
+	std::ostringstream ss;
+	ss << wndRect.left << "," << wndRect.top << "," << width << "," << height;
+	auto wndDataStr = ss.str();
+	dlog_f("Saving console window position & size: %s\n", DL_MAIN, wndDataStr.c_str());
+
+	IniReader::SetDefaultConfigString(IniSection, IniPositionKey, wndDataStr.c_str());
+}
+
+static void __fastcall WriteGameLog(const char* a) {
+	ConsoleWindow::instance().write(a, ConsoleWindow::Source::GAME);
+}
+
+static void __declspec(naked) debug_printf_hook() {
+	__asm {
+		call fo::funcoffs::vsprintf_;
+		pushadc;
+		lea ecx, [esp + 16];
+		call WriteGameLog;
+		popadc;
+		retn;
+	}
+}
+
+void ConsoleWindow::init() {
+	_mode = IniReader::GetIntDefaultConfig(IniSection, IniModeKey, 0);
+	if (_mode == 0) return;
+	if (!AllocConsole()) {
+		dlog_f("Failed to allocate console: 0x%x\n", DL_MAIN, GetLastError());
+		return;
+	}
+	int cp = IniReader::GetIntDefaultConfig(IniSection, IniCodePageKey, 0);
+	if (cp > 0) SetConsoleOutputCP(cp);
+
+	freopen("CONOUT$", "w", stdout); // this allows to print to console via std::cout
+
+	if (_mode & Source::GAME) {
+		std::cout << "Displaying debug_printf output.\n";
+		HookCall(0x4C6F77, debug_printf_hook);
+	}
+	if (_mode & Source::SFALL) {
+		std::cout << "Displaying sfall debug output.\n";
+	}
+	if (_mode & Source::DEBUG_MSG) {
+		std::cout << "Displaying debug_msg output.\n";
+	}
+	if (_mode & Source::DISPLAY_MSG) {
+		std::cout << "Displaying display_msg output.\n";
+	}
+	std::cout << std::endl;
+
+	loadPosition();
+}
+
+ConsoleWindow::~ConsoleWindow() {
+	if (_mode == 0) return;
+
+	savePosition();
+}
+
+void ConsoleWindow::write(const char* message, ConsoleWindow::Source source) {
+	if (!(_mode & source)) return;
+
+	if (source == Source::SFALL && _lastSource != Source::SFALL) {
+		std::cout << "\n"; // To make logs prettier, because debug_msg places newline before the message.
+	}
+	std::cout << message;
+	_lastSource = source;
+}
+
+}

sfall/ConsoleWindow.h

@@ -0,0 +1,55 @@
+/*
+ *    sfall
+ *    Copyright (C) 2008-2023  The sfall team
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the GNU General Public License as published by
+ *    the Free Software Foundation, either version 3 of the License, or
+ *    (at your option) any later version.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    GNU General Public License for more details.
+ *
+ *    You should have received a copy of the GNU General Public License
+ *    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+namespace sfall
+{
+
+class ConsoleWindow {
+public:
+
+	enum Source : int {
+		GAME = 1,
+		SFALL = 2,
+		DEBUG_MSG = 4,
+		DISPLAY_MSG = 8,
+	};
+
+	static ConsoleWindow& instance() { return _instance; }
+
+	ConsoleWindow() {}
+	~ConsoleWindow();
+
+	void init();
+
+	void loadPosition();
+	void savePosition();
+
+	void write(const char* message, Source source);
+
+private:
+	static ConsoleWindow _instance;
+
+	int _mode = 0;
+	Source _lastSource;
+
+	bool tryGetWindow(HWND* wnd);
+};
+
+}

sfall/Logging.cpp

@@ -16,167 +16,24 @@
  *    along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "main.h"
 #include "Logging.h"
+
+#include "main.h"
 #include "FalloutEngine\Fallout2.h"
+#include "ConsoleWindow.h"
 #include "Utils.h"
 
-#ifndef NO_SFALL_DEBUG
-
 #include <fstream>
-#include <iostream>
-#include <sstream>
 
 namespace sfall
 {
 
-enum ConsoleSource : int {
-	GAME = 1,
-	SFALL = 2
-};
-
 static int DebugTypes = 0;
 static std::ofstream Log;
 
 static int LastType = -1;
 static int LastNewLine;
 
-class ConsoleWindow {
-public:
-	static constexpr char* IniSection = "Debugging";
-	static constexpr char* IniModeKey = "ConsoleWindow";
-	static constexpr char* IniPositionKey = "ConsoleWindowData";
-	static constexpr char* IniCodePageKey = "ConsoleCodePage";
-
-	static ConsoleWindow& instance() { return _instance; }
-
-	ConsoleWindow() {}
-	~ConsoleWindow();
-
-	void init();
-
-	void loadPosition();
-	void savePosition();
-
-	void sfallLog(const std::string& a, int type);
-	void falloutLog(const char* a);
-
-private:
-	static ConsoleWindow _instance;
-
-	int _mode = 0;
-	ConsoleSource _lastSource;
-
-	bool tryGetWindow(HWND* wnd);
-};
-
-ConsoleWindow ConsoleWindow::_instance;
-
-bool ConsoleWindow::tryGetWindow(HWND* wnd) {
-	*wnd = GetConsoleWindow();
-	if (!*wnd) {
-		dlogr("Error getting console window.", DL_MAIN);
-		return false;
-	}
-	return true;
-}
-
-void ConsoleWindow::loadPosition() {
-	auto windowDataStr = IniReader::GetStringDefaultConfig(IniSection, IniPositionKey, "");
-	auto windowDataSplit = split(windowDataStr, ',');
-	if (windowDataSplit.size() < 4) return;
-
-	HWND wnd;
-	if (!tryGetWindow(&wnd)) return;
-
-	int windowData[4];
-	for (size_t i = 0; i < 4; i++) {
-		windowData[i] = atoi(windowDataSplit.at(i).c_str());
-	}
-	if (!SetWindowPos(wnd, HWND_TOP, windowData[0], windowData[1], windowData[2], windowData[3], 0)) {
-		dlog_f("Error repositioning console window: 0x%x\n", DL_MAIN, GetLastError());
-	}
-}
-
-void ConsoleWindow::savePosition() {
-	HWND wnd;
-	if (!tryGetWindow(&wnd)) return;
-
-	RECT wndRect;
-	if (!GetWindowRect(wnd, &wndRect)) {
-		dlog_f("Error getting console window position: 0x%x\n", DL_MAIN, GetLastError());
-	}
-	int width = wndRect.right - wndRect.left;
-	int height = wndRect.bottom - wndRect.top;
-	std::ostringstream ss;
-	ss << wndRect.left << "," << wndRect.top << "," << width << "," << height;
-	auto wndDataStr = ss.str();
-	dlog_f("Saving console window position & size: %s\n", DL_MAIN, wndDataStr.c_str());
-
-	IniReader::SetDefaultConfigString(IniSection, IniPositionKey, wndDataStr.c_str());
-}
-
-static void __fastcall PrintToConsole(const char* a) {
-	ConsoleWindow::instance().falloutLog(a);
-}
-
-static void __declspec(naked) debug_printf_hook() {
-	__asm {
-		call fo::funcoffs::vsprintf_;
-		pushadc;
-		lea ecx, [esp + 16];
-		call PrintToConsole;
-		popadc;
-		retn;
-	}
-}
-
-void ConsoleWindow::init() {
-	_mode = IniReader::GetIntDefaultConfig(IniSection, IniModeKey, 0);
-	if (_mode == 0) return;
-	if (!AllocConsole()) {
-		dlog_f("Failed to allocate console: 0x%x\n", DL_MAIN, GetLastError());
-		return;
-	}
-	int cp = IniReader::GetIntDefaultConfig(IniSection, IniCodePageKey, 0);
-	if (cp > 0) SetConsoleOutputCP(cp);
-
-	freopen("CONOUT$", "w", stdout); // this allows to print to console via std::cout
-
-	if (_mode & ConsoleSource::GAME) {
-		std::cout << "Displaying debug_printf output.\n";
-		HookCall(0x4C6F77, debug_printf_hook);
-	}
-	if (_mode & ConsoleSource::SFALL) {
-		std::cout << "Displaying sfall debug output.\n";
-	}
-	std::cout << std::endl;
-
-	loadPosition();
-}
-
-ConsoleWindow::~ConsoleWindow() {
-	if (_mode == 0) return;
-
-	savePosition();
-}
-
-void ConsoleWindow::falloutLog(const char* a) {
-	std::cout << a;
-	_lastSource = ConsoleSource::GAME;
-}
-
-void ConsoleWindow::sfallLog(const std::string& a, int type) {
-	if (!(_mode & ConsoleSource::SFALL)) return;
-
-	if (_lastSource == ConsoleSource::GAME) {
-		std::cout << "\n"; // To make logs prettier, because debug_msg places newline before the message.
-	}
-	std::cout << a;
-	_lastSource = ConsoleSource::SFALL;
-}
-
-
 template <class T>
 static void OutLog(T a, int type, bool newLine = false) {
 	std::ostringstream ss;
@@ -187,7 +44,7 @@ static void OutLog(T a, int type, bool newLine = false) {
 	if (newLine) ss << "\n";
 	std::string str = ss.str();
 
-	ConsoleWindow::instance().sfallLog(str, type);
+	ConsoleWindow::instance().write(str.c_str(), ConsoleWindow::Source::SFALL);
 
 	Log << str;
 	Log.flush();
@@ -280,10 +137,7 @@ void LoggingInit() {
 	if (IniReader::GetIntDefaultConfig("Debugging", "Fixes", 0)) {
 		DebugTypes |= DL_FIX;
 	}
-
-	ConsoleWindow::instance().init();
 }
 
 }
 
-#endif

sfall/ModuleManager.cpp

@@ -23,7 +23,7 @@ void ModuleManager::initAll() {
 	}
 }
 
-ModuleManager& ModuleManager::getInstance() {
+ModuleManager& ModuleManager::instance() {
 	return _instance;
 }
 

sfall/ModuleManager.h

@@ -23,7 +23,7 @@ public:
 		_modules.emplace_back(new T());
 	}
 
-	static ModuleManager& getInstance();
+	static ModuleManager& instance();
 
 private:
 	// disallow copy constructor and copy assignment because we're dealing with unique_ptr's here

sfall/Modules/DebugEditor.cpp

@@ -19,6 +19,7 @@
 #include <vector>
 
 #include "..\main.h"
+#include "..\ConsoleWindow.h"
 #include "..\FalloutEngine\Fallout2.h"
 #include "..\InputFuncs.h"
 //#include "Graphics.h"
@@ -394,6 +395,45 @@ static void __declspec(naked) combat_load_hack() {
 	}
 }
 
+static void __fastcall DuplicateLogToConsole(const char* a, unsigned long displayMsg) {
+	auto& console = ConsoleWindow::instance();
+	auto source = displayMsg ? ConsoleWindow::Source::DISPLAY_MSG : ConsoleWindow::Source::DEBUG_MSG;
+	console.write("\n", source);
+	console.write(a, source);
+}
+
+static void __declspec(naked) op_display_debug_msg_hack() {
+	__asm {
+		mov eax, 0x505224; // "\n"
+		call ds:[FO_VAR_debug_func];
+		mov eax, esi; // actual message
+		call ds:[FO_VAR_debug_func];
+		pushadc;
+		mov ecx, esi;
+		mov edx, [esp + 12];
+		call DuplicateLogToConsole; // duplicate messages to console window
+		popadc;
+		add esp, 4; // eat displayMsg flag
+		pop eax;
+		add eax, 17; // skip to the end of functions
+		jmp eax;
+	}
+}
+
+static void __declspec(naked) op_display_msg_hack() {
+	__asm {
+		push 1; // displayMsg = true
+		jmp op_display_debug_msg_hack;
+	}
+}
+
+static void __declspec(naked) op_debug_msg_hack() {
+	__asm {
+		push 0; // displayMsg = false
+		jmp op_display_debug_msg_hack;
+	}
+}
+
 // Shifts the string one character to the right and inserts a newline control character at the beginning
 static void MoveDebugString(char* messageAddr) {
 	int i = 0;
@@ -445,6 +485,9 @@ static void DebugModePatch() {
 			MakeCall(0x4C703F, debug_log_hack);
 			BlockCall(0x4C7044); // just nop code
 		}
+		// replace calling debug_printf_ with _debug_func, to avoid buffer overflow with messages longer than 260-bytes.
+		MakeCall(0x45540F, op_display_msg_hack);
+		MakeCall(0x45CB4E, op_debug_msg_hack);
 
 		// set the position of the debug window
 		SafeWrite8(0x4DC34D, 15);

sfall/Modules/MiscPatches.cpp

@@ -254,7 +254,7 @@ static void __fastcall SwapHandSlots(fo::GameObject* item, fo::GameObject* &toSl
 		}
 		std::memcpy(dstSlot, &item, 0x14);
 	} else { // swap slots
-		auto hands = fo::var::itemButtonItems;
+		auto& hands = fo::var::itemButtonItems;
 		hands[fo::HandSlot::Left].primaryAttack    = fo::AttackType::ATKTYPE_RWEAPON_PRIMARY;
 		hands[fo::HandSlot::Left].secondaryAttack  = fo::AttackType::ATKTYPE_RWEAPON_SECONDARY;
 		hands[fo::HandSlot::Right].primaryAttack   = fo::AttackType::ATKTYPE_LWEAPON_PRIMARY;

sfall/Modules/Scripting/Arrays.h

@@ -29,7 +29,7 @@ namespace sfall
 namespace script
 {
 
-#define ARRAY_MAX_STRING        (255)  // maximum length of string to be stored as array key or value
+#define ARRAY_MAX_STRING       (1024)  // maximum length of string to be stored as array key or value (including null-terminator)
 #define ARRAY_MAX_SIZE       (100000)  // maximum number of array elements,
                                        // so total maximum memory/disk footprint of one array is: 16 + (ARRAY_MAX_STRING + 8) * ARRAY_MAX_SIZE
 

sfall/SafeWrite.h

@@ -17,6 +17,10 @@ enum CodeType : BYTE {
 	JumpZ     = 0x74, // 0x74 [jz  short ...]
 };
 
+// Macros for quick replacement of assembler opcodes pushad/popad
+#define pushadc __asm push eax __asm push edx __asm push ecx
+#define popadc __asm pop ecx __asm pop edx __asm pop eax
+
 template <typename T>
 void __stdcall SafeWrite(DWORD addr, T data) {
 	DWORD oldProtect;

sfall/ddraw.vcxproj

@@ -345,6 +345,7 @@
   </ItemDefinitionGroup>
   <ItemGroup>
     <ClInclude Include="CheckAddress.h" />
+    <ClInclude Include="ConsoleWindow.h" />
     <ClInclude Include="Delegate.h" />
     <ClInclude Include="FalloutEngine\AsmMacros.h" />
     <ClInclude Include="FalloutEngine\Enums.h" />
@@ -496,6 +497,7 @@
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="CheckAddress.cpp" />
+    <ClCompile Include="ConsoleWindow.cpp" />
     <ClCompile Include="FalloutEngine\EngineUtils.cpp" />
     <ClCompile Include="FalloutEngine\FunctionOffsets.cpp" />
     <ClCompile Include="FalloutEngine\Variables.cpp" />

sfall/ddraw.vcxproj.filters

@@ -452,6 +452,7 @@
     <ClInclude Include="HLSL\L8PixelShader.h">
       <Filter>HLSL</Filter>
     </ClInclude>
+    <ClInclude Include="ConsoleWindow.h" />
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="main.cpp" />
@@ -824,6 +825,7 @@
     <ClCompile Include="Modules\SubModules\DirectDraw.cpp">
       <Filter>Modules\SubModules</Filter>
     </ClCompile>
+    <ClCompile Include="ConsoleWindow.cpp" />
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="version.rc" />

sfall/main.cpp

@@ -75,6 +75,7 @@
 #include "Modules\Unarmed.h"
 #include "Modules\Worldmap.h"
 
+#include "ConsoleWindow.h"
 #include "CRC.h"
 #include "InputFuncs.h"
 #include "Logging.h"
@@ -102,7 +103,7 @@ char falloutConfigName[65];
 static void InitModules() {
 	dlogr("In InitModules", DL_INIT);
 
-	auto& manager = ModuleManager::getInstance();
+	auto& manager = ModuleManager::instance();
 
 	// initialize all modules
 	manager.add<BugFixes>();    // fixes should be applied at the beginning
@@ -214,6 +215,7 @@ static HMODULE SfallInit() {
 	if (!CRC(filepath)) return 0;
 
 	LoggingInit();
+	ConsoleWindow::instance().init();
 
 	// enabling debugging features
 	isDebug = (IniReader::GetIntDefaultConfig("Debugging", "Enable", 0) != 0);

sfall/main.h

@@ -81,10 +81,6 @@ namespace sfall
 // Trap for Debugger
 #define BREAKPOINT __asm int 3
 
-// Macros for quick replacement of assembler opcodes pushad/popad
-#define pushadc __asm push eax __asm push edx __asm push ecx
-#define popadc __asm pop ecx __asm pop edx __asm pop eax
-
 extern bool versionCHI;
 
 extern char falloutConfigName[65];