Fix string overflow in configstrings, replace most info_string usages with std::string

src/engine/client/cg_api.h

@@ -70,9 +70,9 @@ struct cgClientState_t
 	connstate_t connState;
 	int         connectPacketCount;
 	int         clientNum;
-	char        servername[ MAX_STRING_CHARS ];
-	char        updateInfoString[ MAX_STRING_CHARS ];
-	char        messageString[ MAX_STRING_CHARS ];
+	std::string servername;
+	std::string updateInfoString;
+	std::string messageString;
 };
 
 enum class MouseMode

src/engine/client/cg_msgdef.h

@@ -120,6 +120,29 @@ namespace Util {
 			return value;
 		}
 	};
+
+	template<>
+	struct SerializeTraits<cgClientState_t> {
+		static void Write( Writer& stream, const cgClientState_t& clientState ) {
+			stream.Write<int>( ( int ) clientState.connState );
+			stream.Write<int>( clientState.connectPacketCount );
+			stream.Write<int>( clientState.clientNum );
+			stream.Write<std::string>( clientState.servername );
+			stream.Write<std::string>( clientState.updateInfoString );
+			stream.Write<std::string>( clientState.messageString );
+		}
+		static cgClientState_t Read( Reader& stream ) {
+			cgClientState_t clientState;
+			clientState.connState = ( connstate_t ) stream.Read<int>();
+			clientState.connectPacketCount = stream.Read<int>();
+			clientState.clientNum = stream.Read<int>();
+			clientState.servername = stream.Read<std::string>();
+			clientState.updateInfoString = stream.Read<std::string>();
+			clientState.messageString = stream.Read<std::string>();
+
+			return clientState;
+		}
+	};
 }
 
 enum cgameImport_t
@@ -447,7 +470,7 @@ namespace LAN {
 	>;
 	using ResetPingsMsg = IPC::Message<IPC::Id<VM::QVM, CG_LAN_RESETPINGS>, int>;
 	using ServerStatusMsg = IPC::SyncMessage<
-		IPC::Message<IPC::Id<VM::QVM, CG_LAN_SERVERSTATUS>, std::string, int>,
+		IPC::Message<IPC::Id<VM::QVM, CG_LAN_SERVERSTATUS>, std::string>,
 		IPC::Reply<std::string, int>
 	>;
 	using ResetServerStatusMsg = IPC::Message<IPC::Id<VM::QVM, CG_LAN_RESETSERVERSTATUS>>;

src/engine/client/cl_cgame.cpp

@@ -263,11 +263,11 @@ void CL_FillServerCommands(std::vector<std::string>& commands, int start, int en
 
 	if ( end > clc.serverCommandSequence )
 	{
-		Sys::Drop( "CL_FillServerCommand: requested a command not received" );
+		Sys::Drop( "CL_FillServerCommand: requested command not received" );
 	}
 
-	for (int i = start; i <= end; i++) {
-		const char* s = clc.serverCommands[ i & ( MAX_RELIABLE_COMMANDS - 1 ) ];
+	for (int i = start; i < end; i++) {
+		const char* s = clc.serverCommands[i].c_str();
 
 		std::string cmdText = s;
 		if (CL_HandleServerCommand(s, cmdText)) {
@@ -312,7 +312,7 @@ bool CL_GetSnapshot( int snapshotNumber, ipcSnapshot_t *snapshot )
 	snapshot->ps = clSnap->ps;
 	snapshot->b.entities = clSnap->entities;
 
-	CL_FillServerCommands(snapshot->b.serverCommands, clc.lastExecutedServerCommand + 1, clSnap->serverCommandNum);
+	CL_FillServerCommands(snapshot->b.serverCommands, clc.lastExecutedServerCommand, clSnap->serverCommandNum);
 	clc.lastExecutedServerCommand = clSnap->serverCommandNum;
 
 	return true;
@@ -831,7 +831,7 @@ void CL_SetCGameTime()
 	if ( cl.snap.serverTime < cl.oldFrameServerTime )
 	{
 		// Ridah, if this is a localhost, then we are probably loading a savegame
-		if ( !Q_stricmp( cls.servername, "loopback" ) )
+		if ( cls.servername == "loopback" )
 		{
 			// do nothing?
 			CL_FirstSnapshot();
@@ -1028,9 +1028,9 @@ void CGameVM::CGameRocketFrame()
 	cgClientState_t state;
 	state.connectPacketCount = clc.connectPacketCount;
 	state.connState = cls.state;
-	Q_strncpyz( state.servername, cls.servername, sizeof( state.servername ) );
-	Q_strncpyz( state.updateInfoString, cls.updateInfoString, sizeof( state.updateInfoString ) );
-	Q_strncpyz( state.messageString, clc.serverMessage, sizeof( state.messageString ) );
+	state.servername = cls.servername;
+	state.updateInfoString = cls.updateInfoString;
+	state.messageString = clc.serverMessage;
 	state.clientNum = cl.snap.ps.clientNum;
 	this->SendMsg<CGameRocketFrameMsg>(state);
 }
@@ -1437,17 +1437,14 @@ void CGameVM::QVMSyscall(int syscallNum, Util::Reader& reader, IPC::Channel& cha
 			break;
 
 		case CG_LAN_SERVERSTATUS:
-			IPC::HandleMsg<LAN::ServerStatusMsg>(channel, std::move(reader), [this] (const std::string& serverAddress, int len, std::string& status, int& res) {
-				std::unique_ptr<char[]> buffer(new char[len]);
-				buffer[0] = '\0';
-				res = CL_ServerStatus(serverAddress.c_str(), buffer.get(), len);
-				status.assign(buffer.get());
+			IPC::HandleMsg<LAN::ServerStatusMsg>(channel, std::move(reader), [this] (const std::string& serverAddress, std::string& status, int& res) {
+				res = CL_ServerStatus(serverAddress, status);
 			});
 			break;
 
 		case CG_LAN_RESETSERVERSTATUS:
 			IPC::HandleMsg<LAN::ResetServerStatusMsg>(channel, std::move(reader), [this] {
-				CL_ServerStatus(nullptr, nullptr, 0);
+				CL_ServerStatusReset();
 			});
 			break;
 

src/engine/client/cl_download.cpp

@@ -49,9 +49,9 @@ void CL_ClearStaticDownload()
 {
 	downloadLogger.Debug("Clearing the download info");
 	cls.downloadRestart = false;
-	cls.downloadTempName[ 0 ] = '\0';
-	cls.downloadName[ 0 ] = '\0';
-	cls.originalDownloadName[ 0 ] = '\0';
+	cls.downloadTempName.clear();
+	cls.downloadName.clear();
+	cls.originalDownloadName.clear();
 }
 
 /*
@@ -121,23 +121,24 @@ Requests a file to download from the server.  Stores it in the current
 game directory.
 =================
 */
-static void CL_BeginDownload( const char *localName, const char *remoteName )
+static void CL_BeginDownload( const std::string& localName, const std::string& remoteName )
 {
 	downloadLogger.Debug("Requesting the download of '%s', with remote name '%s'", localName, remoteName);
 
-	Q_strncpyz( cls.downloadName, localName, sizeof( cls.downloadName ) );
-	Com_sprintf( cls.downloadTempName, sizeof( cls.downloadTempName ), "%s.tmp", localName );
+	cls.downloadName = localName;
+	cls.downloadTempName = localName; 
+	cls.downloadTempName += ".tmp";
 
 	// Set so UI gets access to it
-	Cvar_Set( "cl_downloadName", remoteName );
+	Cvar_Set( "cl_downloadName", remoteName.c_str() );
 	Cvar_Set( "cl_downloadSize", "0" );
-	cl_downloadCount.Set(0);
+	cl_downloadCount.Set( 0 );
 	Cvar_SetValue( "cl_downloadTime", cls.realtime );
 
 	clc.downloadBlock = 0; // Starting new file
 	clc.downloadCount = 0;
 
-	CL_AddReliableCommand( va( "download %s", Cmd_QuoteString( remoteName ) ) );
+	CL_AddReliableCommand( Str::Format( "download %s", Cmd_QuoteString( remoteName.c_str() ) ).c_str() );
 }
 
 /*
@@ -149,49 +150,39 @@ A download completed or failed
 */
 static void CL_NextDownload()
 {
-	char *s;
-	char *remoteName, *localName;
-
 	// We are looking to start a download here
-	if ( *clc.downloadList )
+	if ( clc.downloadList.size() )
 	{
-		downloadLogger.Debug("CL_NextDownload downloadList is '%s'", clc.downloadList);
-		s = clc.downloadList;
+		downloadLogger.Debug( "CL_NextDownload downloadList is '%s'", clc.downloadList );
 
 		// format is:
 		//  @remotename@localname@remotename@localname, etc.
 
-		if ( *s == '@' )
-		{
-			s++;
-		}
-
-		remoteName = s;
+		size_t offset  = clc.downloadList[0] == '@' ? 1 : 0;
+		size_t offset2 = clc.downloadList.find( "@", offset + 1 );
+		const std::string remoteName = clc.downloadList.substr( offset, offset2 - offset );
 
-		if ( ( s = strchr( s, '@' ) ) == nullptr )
+		if ( offset2 == std::string::npos )
 		{
 			CL_DownloadsComplete();
 			return;
 		}
 
-		*s++ = 0;
-		localName = s;
+		offset = offset2 + 1;
+		offset2 = clc.downloadList.find( "@", offset2 + 1 );
 
-		if ( ( s = strchr( s, '@' ) ) != nullptr )
-		{
-			*s++ = 0;
-		}
-		else
-		{
-			s = localName + strlen( localName );  // point at the nul byte
-		}
+		const std::string localName = clc.downloadList.substr( offset, offset2 - 1 );
 
 		CL_BeginDownload( localName, remoteName );
 
 		cls.downloadRestart = true;
 
 		// move over the rest
-		memmove( clc.downloadList, s, strlen( s ) + 1 );
+		if ( offset2 == std::string::npos ) {
+			clc.downloadList.clear();
+		} else {
+			clc.downloadList = clc.downloadList.substr( offset2 );
+		}
 
 		return;
 	}
@@ -215,17 +206,18 @@ void CL_InitDownloads()
 	clc.bWWWDlAborting = false;
 	CL_ClearStaticDownload();
 
-	if ( cl_allowDownload->integer )
+	if ( cl_allowDownload->integer ) {
 		FS_DeletePaksWithBadChecksum();
+	}
 
 	// reset the redirect checksum tracking
-	clc.redirectedList[ 0 ] = '\0';
+	clc.redirectedList.clear();
 
-	if ( cl_allowDownload->integer && FS_ComparePaks( clc.downloadList, sizeof( clc.downloadList ) ) )
+	if ( cl_allowDownload->integer && FS_ComparePaks( clc.downloadList ) )
 	{
-		downloadLogger.Debug("Need paks: '%s'", clc.downloadList);
+		downloadLogger.Debug( "Need paks: '%s'", clc.downloadList );
 
-		if ( *clc.downloadList )
+		if ( clc.downloadList.size() )
 		{
 			// if autodownloading is not enabled on the server
 			cls.state = connstate_t::CA_DOWNLOADING;
@@ -277,24 +269,17 @@ void CL_WWWDownload()
 		// taken from CL_ParseDownload
 		clc.download = 0;
 
-		FS_SV_Rename( cls.downloadTempName, cls.originalDownloadName );
+		FS_SV_Rename( cls.downloadTempName.c_str(), cls.originalDownloadName.c_str() );
 
-		*cls.downloadTempName = *cls.downloadName = 0;
+		cls.downloadTempName.clear();
+		cls.downloadName.clear();
 		Cvar_Set( "cl_downloadName", "" );
 
 		CL_AddReliableCommand( "wwwdl done" );
 
 		// tracking potential web redirects leading us to wrong checksum - only works in connected mode
-		if ( strlen( clc.redirectedList ) + strlen( cls.originalDownloadName ) + 1 >= sizeof( clc.redirectedList ) )
-		{
-			// just to be safe
-			Log::Warn( "redirectedList overflow (%s)", clc.redirectedList );
-		}
-		else
-		{
-			strcat( clc.redirectedList, "@" );
-			strcat( clc.redirectedList, cls.originalDownloadName );
-		}
+		clc.redirectedList += "@";
+		clc.redirectedList += cls.originalDownloadName;
 	}
 	else
 	{
@@ -321,19 +306,13 @@ this indicates that the redirect setup is broken, and next dl attempt should NOT
 */
 bool CL_WWWBadChecksum( const char *pakname )
 {
-	if ( strstr( clc.redirectedList, va( "@%s@", pakname ) ) )
+	if ( clc.redirectedList.find( Str::Format( "@%s@", pakname ) ) != std::string::npos )
 	{
 		Log::Warn("file %s obtained through download redirect has wrong checksum"
 		              "\tthis likely means the server configuration is broken", pakname );
 
-		if ( strlen( clc.badChecksumList ) + strlen( pakname ) + 1 >= sizeof( clc.badChecksumList ) )
-		{
-			Log::Warn("badChecksumList overflowed (%s)", clc.badChecksumList );
-			return false;
-		}
-
-		strcat( clc.badChecksumList, "@" );
-		strcat( clc.badChecksumList, pakname );
+		clc.badChecksumList += "@";
+		clc.badChecksumList += pakname;
 		Log::Debug( "bad checksums: %s", clc.badChecksumList );
 		return true;
 	}
@@ -354,7 +333,7 @@ void CL_ParseDownload( msg_t *msg )
 	unsigned char data[ MAX_MSGLEN ];
 	int           block;
 
-	if ( !*cls.downloadTempName )
+	if ( cls.downloadTempName.empty() )
 	{
 		Log::Notice( "Server sending download, but no download was requested" );
 		// Eat the packet anyway
@@ -386,29 +365,29 @@ void CL_ParseDownload( msg_t *msg )
 		if ( !clc.bWWWDl )
 		{
 			// server is sending us a www download
-			Q_strncpyz( cls.originalDownloadName, cls.downloadName, sizeof( cls.originalDownloadName ) );
-			Q_strncpyz( cls.downloadName, MSG_ReadString( msg ), sizeof( cls.downloadName ) );
+			cls.originalDownloadName = cls.downloadName;
+			cls.downloadName = MSG_ReadString( msg );
+
 			clc.downloadSize = MSG_ReadLong( msg );
 			int basePathLen = MSG_ReadLong( msg );
 
-			downloadLogger.Debug("Server sent us a new WWW DL '%s', size %i, prefix len %i",
-			                     cls.downloadName, clc.downloadSize, basePathLen);
+			downloadLogger.Debug( "Server sent us a new WWW DL '%s', size %i, prefix len %i",
+			                     cls.downloadName, clc.downloadSize, basePathLen );
 
 			Cvar_SetValue( "cl_downloadSize", clc.downloadSize );
 			clc.bWWWDl = true; // activate wwwdl client loop
 			CL_AddReliableCommand( "wwwdl ack" );
 			cls.state = connstate_t::CA_DOWNLOADING;
 
 			// make sure the server is not trying to redirect us again on a bad checksum
-			if ( strstr( clc.badChecksumList, va( "@%s", cls.originalDownloadName ) ) )
-			{
+			if ( clc.badChecksumList.find( Str::Format( "@%s", cls.originalDownloadName ) ) != std::string::npos ) {
 				Log::Notice( "refusing redirect to %s by server (bad checksum)", cls.downloadName );
 				CL_AddReliableCommand( "wwwdl fail" );
 				clc.bWWWDlAborting = true;
 				return;
 			}
 
-			if ( !DL_BeginDownload( cls.downloadTempName, cls.downloadName, basePathLen ) )
+			if ( !DL_BeginDownload( cls.downloadTempName.c_str(), cls.downloadName.c_str(), basePathLen ) )
 			{
 				// setting bWWWDl to false after sending the wwwdl fail doesn't work
 				// not sure why, but I suspect we have to eat all remaining block -1 that the server has sent us
@@ -424,7 +403,6 @@ void CL_ParseDownload( msg_t *msg )
 		else
 		{
 			// server keeps sending that message till we ack it, eat and ignore
-			//MSG_ReadLong( msg );
 			MSG_ReadString( msg );
 			MSG_ReadLong( msg );
 			MSG_ReadLong( msg );
@@ -442,7 +420,7 @@ void CL_ParseDownload( msg_t *msg )
 
 		if ( clc.downloadSize < 0 )
 		{
-			Sys::Drop( "%s", MSG_ReadString( msg ) );
+			Sys::Drop( MSG_ReadString( msg ) );
 		}
 	}
 
@@ -466,7 +444,7 @@ void CL_ParseDownload( msg_t *msg )
 	// open the file if not opened yet
 	if ( !clc.download )
 	{
-		clc.download = FS_SV_FOpenFileWrite( cls.downloadTempName );
+		clc.download = FS_SV_FOpenFileWrite( cls.downloadTempName.c_str() );
 
 		if ( !clc.download )
 		{
@@ -500,10 +478,11 @@ void CL_ParseDownload( msg_t *msg )
 			clc.download = 0;
 
 			// rename the file
-			FS_SV_Rename( cls.downloadTempName, cls.downloadName );
+			FS_SV_Rename( cls.downloadTempName.c_str(), cls.downloadName.c_str() );
 		}
 
-		*cls.downloadTempName = *cls.downloadName = 0;
+		cls.downloadTempName.clear();
+		cls.downloadName.clear();
 		Cvar_Set( "cl_downloadName", "" );
 
 		// send intentions now