SP: Fix use-after-free on repeat GetString() calls with cvars in ICARUS

We can't use an std::string, as pointers to its content become invalidated when the content is modified. Using a 1KB buffer instead lets us avoid re-allocations at the cost of wasting some space, but that should be okay.
bst789 · Apr 16, 2023 · f3fbe80 · f3fbe80
1 parent c55d81a
commit f3fbe80
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 12 deletions.
diff --git a/code/game/Q3_Interface.cpp b/code/game/Q3_Interface.cpp
@@ -10570,11 +10570,14 @@ int		CQuake3GameInterface::GetString( int entID, const char *name, char **value
 
 	if( strlen(name) > 5 && Q_stricmpn(name, "cvar_", 5) )
 	{
-		char cvarbuf[MAX_STRING_CHARS];
-
-		gi.Cvar_VariableStringBuffer(name+5, cvarbuf, sizeof(cvarbuf));
-		m_cvars[name+5] = cvarbuf;
-		*value = &m_cvars[name+5][0];
+		const char* cvar_name = name + 5;
+		// by allocating and then re-using the same sufficiently large buffer,
+		// we ensure that pointers to it never become invalid,
+		// so we can support expressions using the same cvar twice,
+		// e.g. if(get(cvar_x) == get(cvar_x))
+		std::array<char, MAX_STRING_CHARS>& buf = m_cvars[cvar_name];
+		gi.Cvar_VariableStringBuffer(cvar_name, buf.data(), buf.size());
+		*value = buf.data();
 		return true;
 	}
 

diff --git a/code/game/Q3_Interface.h b/code/game/Q3_Interface.h
@@ -23,6 +23,7 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.
 #ifndef __Q3_INTERFACE__
 #define __Q3_INTERFACE__
 
+#include <array>
 #include "../icarus/IcarusInterface.h"
 #include "bg_public.h"
 #include "g_shared.h"
@@ -558,6 +559,8 @@ typedef std::map < std::string, pscript_t* >	scriptlist_t;
 // STL map type definitions for the variable containers.
 typedef std::map < std::string, std::string >		varString_m;
 typedef std::map < std::string, float >		varFloat_m;
+// For cases where we need to re-use a buffer without invalidating it
+typedef std::map < std::string, std::array< char, MAX_STRING_CHARS > >		varStringBuf_m;
 
 
 // The Quake 3 Game Interface Class for Quake3 and Icarus to use.
@@ -577,7 +580,7 @@ class CQuake3GameInterface : public IGameInterface
 	varString_m		m_varVectors;
 	int				m_numVariables;
 
-	varString_m		m_cvars;
+	varStringBuf_m		m_cvars;
 
 	int				m_entFilter;
 

diff --git a/codeJK2/game/Q3_Interface.cpp b/codeJK2/game/Q3_Interface.cpp
@@ -8599,11 +8599,14 @@ static int Q3_GetString( int entID, int type, const char *name, char **value )
 
 	if( strlen(name) > 5 && Q_stricmpn(name, "cvar_", 5) )
 	{
-		char cvarbuf[MAX_STRING_CHARS];
-
-		gi.Cvar_VariableStringBuffer(name+5, cvarbuf, sizeof(cvarbuf));
-		ICARUS_CvarList[name+5] = cvarbuf;
-		*value = &ICARUS_CvarList[name+5][0];
+		const char* cvar_name = name + 5;
+		// by allocating and then re-using the same sufficiently large buffer,
+		// we ensure that pointers to it never become invalid,
+		// so we can support expressions using the same cvar twice,
+		// e.g. if(get(cvar_x) == get(cvar_x))
+		std::array<char, MAX_STRING_CHARS>& buf = m_cvars[cvar_name];
+		gi.Cvar_VariableStringBuffer(cvar_name, buf.data(), buf.size());
+		*value = buf.data();
 		return true;
 	}
 

diff --git a/codeJK2/game/Q3_Registers.h b/codeJK2/game/Q3_Registers.h
@@ -27,6 +27,7 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.
 
 #include <string>
 #include <map>
+#include <array>
 
 typedef std::map < std::string, std::string >		varString_m;
 typedef std::map < std::string, float >		varFloat_m;

diff --git a/codeJK2/game/g_local.h b/codeJK2/game/g_local.h
@@ -636,7 +636,7 @@ typedef struct pscript_s
 
 typedef std::map < std::string, int, std::less<std::string> >		entlist_t;
 typedef std::map < std::string, pscript_t*, std::less<std::string> >	bufferlist_t;
-typedef std::map < std::string, std::string >		cvarlist_t;
+typedef std::map < std::string, std::array<char, MAX_STRING_CHARS> >		cvarlist_t;
 
 
 extern char *G_NewString( const char *string );