Validate skin names when loading, refactor loading

Prevent skins with invalid names from being loaded/downloaded.

Improve log messages when skins cannot be loaded.

Remove obsolete check for duplicate skins, as the storage handles duplicate files already.

src/game/client/components/skins.cpp

@@ -1,6 +1,7 @@
 /* (c) Magnus Auvinen. See licence.txt in the root of the distribution for more information. */
 /* If you are missing that file, acquire a complete release at teeworlds.com.                */
 
+#include <base/log.h>
 #include <base/math.h>
 #include <base/system.h>
 
@@ -67,27 +68,30 @@ struct SSkinScanUser
 
 int CSkins::SkinScan(const char *pName, int IsDir, int DirType, void *pUser)
 {
-	auto *pUserReal = (SSkinScanUser *)pUser;
+	auto *pUserReal = static_cast<SSkinScanUser *>(pUser);
 	CSkins *pSelf = pUserReal->m_pThis;
 
-	if(IsDir || !str_endswith(pName, ".png"))
+	if(IsDir)
 		return 0;
 
-	char aNameWithoutPng[128];
+	const char *pSuffix = str_endswith(pName, ".png");
-	str_copy(aNameWithoutPng, pName);
+	if(pSuffix == nullptr)
-	aNameWithoutPng[str_length(aNameWithoutPng) - 4] = 0;
-
-	if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(aNameWithoutPng))
 		return 0;
 
-	// Don't add duplicate skins (one from user's config directory, other from
+	char aSkinName[IO_MAX_PATH_LENGTH];
-	// client itself)
+	str_truncate(aSkinName, sizeof(aSkinName), pName, pSuffix - pName);
-	if(pSelf->m_Skins.find(aNameWithoutPng) != pSelf->m_Skins.end())
+	if(!CSkin::IsValidName(aSkinName))
+	{
+		log_error("skins", "Skin name is not valid: %s", aSkinName);
+		return 0;
+	}
+
+	if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(aSkinName))
 		return 0;
 
-	char aBuf[IO_MAX_PATH_LENGTH];
+	char aPath[IO_MAX_PATH_LENGTH];
-	str_format(aBuf, sizeof(aBuf), "skins/%s", pName);
+	str_format(aPath, sizeof(aPath), "skins/%s", pName);
-	pSelf->LoadSkin(aNameWithoutPng, aBuf, DirType);
+	pSelf->LoadSkin(aSkinName, aPath, DirType);
 	pUserReal->m_SkinLoadedFunc((int)pSelf->m_Skins.size());
 	return 0;
 }
@@ -131,17 +135,15 @@ const CSkin *CSkins::LoadSkin(const char *pName, const char *pPath, int DirType)
 {
 	CImageInfo Info;
 	if(!LoadSkinPNG(Info, pName, pPath, DirType))
-		return 0;
+		return nullptr;
 	return LoadSkin(pName, Info);
 }
 
 bool CSkins::LoadSkinPNG(CImageInfo &Info, const char *pName, const char *pPath, int DirType)
 {
-	char aBuf[512];
 	if(!Graphics()->LoadPNG(&Info, pPath, DirType))
 	{
-		str_format(aBuf, sizeof(aBuf), "failed to load skin from %s", pName);
+		log_error("skins", "Failed to load skin PNG: %s", pName);
-		Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf);
 		return false;
 	}
 	return true;
@@ -149,18 +151,14 @@ bool CSkins::LoadSkinPNG(CImageInfo &Info, const char *pName, const char *pPath,
 
 const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info)
 {
-	char aBuf[512];
-
 	if(!Graphics()->CheckImageDivisibility(pName, Info, g_pData->m_aSprites[SPRITE_TEE_BODY].m_pSet->m_Gridx, g_pData->m_aSprites[SPRITE_TEE_BODY].m_pSet->m_Gridy, true))
 	{
-		str_format(aBuf, sizeof(aBuf), "skin failed image divisibility: %s", pName);
+		log_error("skins", "Skin failed image divisibility: %s", pName);
-		Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf);
 		return nullptr;
 	}
 	if(!Graphics()->IsImageFormatRGBA(pName, Info))
 	{
-		str_format(aBuf, sizeof(aBuf), "skin format is not RGBA: %s", pName);
+		log_error("skins", "Skin format is not RGBA: %s", pName);
-		Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf);
 		return nullptr;
 	}
 
@@ -297,11 +295,9 @@ const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info)
 
 	Graphics()->FreePNG(&Info);
 
-	// set skin data
 	if(g_Config.m_Debug)
 	{
-		str_format(aBuf, sizeof(aBuf), "load skin %s", Skin.GetName());
+		log_trace("skins", "Loaded skin %s", Skin.GetName());
-		Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf);
 	}
 
 	auto &&pSkin = std::make_unique<CSkin>(std::move(Skin));
@@ -366,22 +362,24 @@ const CSkin *CSkins::Find(const char *pName)
 
 const CSkin *CSkins::FindOrNullptr(const char *pName, bool IgnorePrefix)
 {
-	const char *pSkinPrefix = m_aEventSkinPrefix[0] ? m_aEventSkinPrefix : g_Config.m_ClSkinPrefix;
 	if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(pName))
 	{
 		return nullptr;
 	}
-	else if(pSkinPrefix && pSkinPrefix[0] && !IgnorePrefix)
+
+	const char *pSkinPrefix = m_aEventSkinPrefix[0] != '\0' ? m_aEventSkinPrefix : g_Config.m_ClSkinPrefix;
+	if(!IgnorePrefix && pSkinPrefix[0] != '\0')
 	{
-		char aBuf[24];
+		char aNameWithPrefix[48]; // Larger than skin name length to allow IsValidName to check if it's too long
-		str_format(aBuf, sizeof(aBuf), "%s_%s", pSkinPrefix, pName);
+		str_format(aNameWithPrefix, sizeof(aNameWithPrefix), "%s_%s", pSkinPrefix, pName);
 		// If we find something, use it, otherwise fall back to normal skins.
-		const auto *pResult = FindImpl(aBuf);
+		const auto *pResult = FindImpl(aNameWithPrefix);
 		if(pResult != nullptr)
 		{
 			return pResult;
 		}
 	}
+
 	return FindImpl(pName);
 }
 
@@ -397,7 +395,7 @@ const CSkin *CSkins::FindImpl(const char *pName)
 	if(!g_Config.m_ClDownloadSkins)
 		return nullptr;
 
-	if(str_find(pName, "/") != 0)
+	if(!CSkin::IsValidName(pName))
 		return nullptr;
 
 	const auto SkinDownloadIt = m_DownloadSkins.find(pName);
@@ -423,9 +421,9 @@ const CSkin *CSkins::FindImpl(const char *pName)
 
 	CDownloadSkin Skin{pName};
 
-	char aUrl[IO_MAX_PATH_LENGTH];
 	char aEscapedName[256];
 	EscapeUrl(aEscapedName, sizeof(aEscapedName), pName);
+	char aUrl[IO_MAX_PATH_LENGTH];
 	str_format(aUrl, sizeof(aUrl), "%s%s.png", g_Config.m_ClDownloadCommunitySkins != 0 ? g_Config.m_ClSkinCommunityDownloadUrl : g_Config.m_ClSkinDownloadUrl, aEscapedName);
 
 	char aBuf[IO_MAX_PATH_LENGTH];