From 8f37729f5b3d39e1adaa5f51fca1dc705d12edce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 3 Oct 2024 22:09:20 +0200 Subject: [PATCH] Improve map image validation - Add check for map image data being loaded from map to avoid null pointer access. - Add check for size of map image data to avoid out-of-bounds reads. - Add check for map layer item not being found to avoid null pointer access. - Fix inconsistent handling of layers with invalid images. In some cases only `-1` was considered non-textured whereas all invalid values should be. - Ensure correct texture is always used for game layers instead of considering the image property of game layers which cannot be set via the map editor. - Skip loading images which are not used by any quads or tiles layers. - Replace member variable `m_aTextureUsedByTileOrQuadLayerFlag` with local variable. - Mark pointers as `const` when possible. - Avoid C-style casts. --- src/game/client/components/mapimages.cpp | 61 +++++++++++++++--------- src/game/client/components/mapimages.h | 1 - src/game/client/components/maplayers.cpp | 36 +++++++------- 3 files changed, 57 insertions(+), 41 deletions(-) diff --git a/src/game/client/components/mapimages.cpp b/src/game/client/components/mapimages.cpp index 56997867f..aea75a40a 100644 --- a/src/game/client/components/mapimages.cpp +++ b/src/game/client/components/mapimages.cpp @@ -37,8 +37,6 @@ CMapImages::CMapImages(int TextureSize) mem_zero(m_aEntitiesIsLoaded, sizeof(m_aEntitiesIsLoaded)); m_SpeedupArrowIsLoaded = false; - mem_zero(m_aTextureUsedByTileOrQuadLayerFlag, sizeof(m_aTextureUsedByTileOrQuadLayerFlag)); - str_copy(m_aEntitiesPath, "editor/entities_clear"); static_assert(std::size(gs_apModEntitiesNames) == MAP_IMAGE_MOD_TYPE_COUNT, "Mod name string count is not equal to mod type count"); @@ -61,41 +59,44 @@ void CMapImages::OnMapLoadImpl(class CLayers *pLayers, IMap *pMap) // unload all textures for(int i = 0; i < m_Count; i++) { - Graphics()->UnloadTexture(&(m_aTextures[i])); - m_aTextureUsedByTileOrQuadLayerFlag[i] = 0; + Graphics()->UnloadTexture(&m_aTextures[i]); } - m_Count = 0; int Start; pMap->GetType(MAPITEMTYPE_IMAGE, &Start, &m_Count); - m_Count = clamp(m_Count, 0, MAX_MAPIMAGES); - for(int g = 0; g < pLayers->NumGroups(); g++) + unsigned char aTextureUsedByTileOrQuadLayerFlag[MAX_MAPIMAGES] = {0}; // 0: nothing, 1(as flag): tile layer, 2(as flag): quad layer + for(int GroupIndex = 0; GroupIndex < pLayers->NumGroups(); GroupIndex++) { - CMapItemGroup *pGroup = pLayers->GetGroup(g); + const CMapItemGroup *pGroup = pLayers->GetGroup(GroupIndex); if(!pGroup) { continue; } - for(int l = 0; l < pGroup->m_NumLayers; l++) + for(int LayerIndex = 0; LayerIndex < pGroup->m_NumLayers; LayerIndex++) { - CMapItemLayer *pLayer = pLayers->GetLayer(pGroup->m_StartLayer + l); + const CMapItemLayer *pLayer = pLayers->GetLayer(pGroup->m_StartLayer + LayerIndex); + if(!pLayer) + { + continue; + } + if(pLayer->m_Type == LAYERTYPE_TILES) { - CMapItemLayerTilemap *pTLayer = (CMapItemLayerTilemap *)pLayer; - if(pTLayer->m_Image >= 0 && pTLayer->m_Image < m_Count) + const CMapItemLayerTilemap *pLayerTilemap = reinterpret_cast(pLayer); + if(pLayerTilemap->m_Image >= 0 && pLayerTilemap->m_Image < m_Count) { - m_aTextureUsedByTileOrQuadLayerFlag[pTLayer->m_Image] |= 1; + aTextureUsedByTileOrQuadLayerFlag[pLayerTilemap->m_Image] |= 1; } } else if(pLayer->m_Type == LAYERTYPE_QUADS) { - CMapItemLayerQuads *pQLayer = (CMapItemLayerQuads *)pLayer; - if(pQLayer->m_Image >= 0 && pQLayer->m_Image < m_Count) + const CMapItemLayerQuads *pLayerQuads = reinterpret_cast(pLayer); + if(pLayerQuads->m_Image >= 0 && pLayerQuads->m_Image < m_Count) { - m_aTextureUsedByTileOrQuadLayerFlag[pQLayer->m_Image] |= 2; + aTextureUsedByTileOrQuadLayerFlag[pLayerQuads->m_Image] |= 2; } } } @@ -107,8 +108,14 @@ void CMapImages::OnMapLoadImpl(class CLayers *pLayers, IMap *pMap) bool ShowWarning = false; for(int i = 0; i < m_Count; i++) { - const int LoadFlag = (((m_aTextureUsedByTileOrQuadLayerFlag[i] & 1) != 0) ? TextureLoadFlag : 0) | (((m_aTextureUsedByTileOrQuadLayerFlag[i] & 2) != 0) ? 0 : (Graphics()->HasTextureArraysSupport() ? IGraphics::TEXLOAD_NO_2D_TEXTURE : 0)); - const CMapItemImage_v2 *pImg = (CMapItemImage_v2 *)pMap->GetItem(Start + i); + if(aTextureUsedByTileOrQuadLayerFlag[i] == 0) + { + // skip loading unused images + continue; + } + + const int LoadFlag = (((aTextureUsedByTileOrQuadLayerFlag[i] & 1) != 0) ? TextureLoadFlag : 0) | (((aTextureUsedByTileOrQuadLayerFlag[i] & 2) != 0) ? 0 : (Graphics()->HasTextureArraysSupport() ? IGraphics::TEXLOAD_NO_2D_TEXTURE : 0)); + const CMapItemImage_v2 *pImg = static_cast(pMap->GetItem(Start + i)); const char *pName = pMap->GetDataString(pImg->m_ImageName); if(pName == nullptr || pName[0] == '\0') @@ -151,10 +158,20 @@ void CMapImages::OnMapLoadImpl(class CLayers *pLayers, IMap *pMap) ImageInfo.m_Height = pImg->m_Height; ImageInfo.m_Format = CImageInfo::FORMAT_RGBA; ImageInfo.m_pData = static_cast(pMap->GetData(pImg->m_ImageData)); - char aTexName[IO_MAX_PATH_LENGTH]; - str_format(aTexName, sizeof(aTexName), "embedded: %s", pName); - m_aTextures[i] = Graphics()->LoadTextureRaw(ImageInfo, LoadFlag, aTexName); - pMap->UnloadData(pImg->m_ImageData); + if(ImageInfo.m_pData && (size_t)pMap->GetDataSize(pImg->m_ImageData) >= ImageInfo.DataSize()) + { + char aTexName[IO_MAX_PATH_LENGTH]; + str_format(aTexName, sizeof(aTexName), "embedded: %s", pName); + m_aTextures[i] = Graphics()->LoadTextureRaw(ImageInfo, LoadFlag, aTexName); + pMap->UnloadData(pImg->m_ImageData); + } + else + { + pMap->UnloadData(pImg->m_ImageData); + log_error("mapimages", "Failed to load map image %d: failed to load data.", i); + ShowWarning = true; + continue; + } } pMap->UnloadData(pImg->m_ImageName); ShowWarning = ShowWarning || m_aTextures[i].IsNullTexture(); diff --git a/src/game/client/components/mapimages.h b/src/game/client/components/mapimages.h index a866b0d1b..45709182b 100644 --- a/src/game/client/components/mapimages.h +++ b/src/game/client/components/mapimages.h @@ -37,7 +37,6 @@ class CMapImages : public CComponent friend class CMenuBackground; IGraphics::CTextureHandle m_aTextures[MAX_MAPIMAGES]; - int m_aTextureUsedByTileOrQuadLayerFlag[MAX_MAPIMAGES]; // 0: nothing, 1(as flag): tile layer, 2(as flag): quad layer int m_Count; char m_aEntitiesPath[IO_MAX_PATH_LENGTH]; diff --git a/src/game/client/components/maplayers.cpp b/src/game/client/components/maplayers.cpp index 30a6ad649..bcffc0086 100644 --- a/src/game/client/components/maplayers.cpp +++ b/src/game/client/components/maplayers.cpp @@ -386,15 +386,8 @@ void CMapLayers::OnMapLoad() if(pLayer->m_Type == LAYERTYPE_TILES && Graphics()->IsTileBufferingEnabled()) { - bool DoTextureCoords = false; CMapItemLayerTilemap *pTMap = (CMapItemLayerTilemap *)pLayer; - if(pTMap->m_Image == -1) - { - if(IsEntityLayer) - DoTextureCoords = true; - } - else - DoTextureCoords = true; + const bool DoTextureCoords = IsEntityLayer || (pTMap->m_Image >= 0 && pTMap->m_Image < m_pImages->Num()); int DataIndex = 0; unsigned int TileSize = 0; @@ -740,7 +733,7 @@ void CMapLayers::OnMapLoad() m_vpQuadLayerVisuals.push_back(new SQuadLayerVisuals()); SQuadLayerVisuals *pQLayerVisuals = m_vpQuadLayerVisuals.back(); - bool Textured = (pQLayer->m_Image != -1); + const bool Textured = pQLayer->m_Image >= 0 && pQLayer->m_Image < m_pImages->Num(); vtmpQuads.clear(); vtmpQuadsTextured.clear(); @@ -1543,15 +1536,18 @@ void CMapLayers::OnRender() if(pLayer->m_Type == LAYERTYPE_TILES) { CMapItemLayerTilemap *pTMap = (CMapItemLayerTilemap *)pLayer; - if(pTMap->m_Image < 0 || pTMap->m_Image >= m_pImages->Num()) + if(IsGameLayer) { - if(!IsGameLayer) - Graphics()->TextureClear(); - else - Graphics()->TextureSet(m_pImages->GetEntities(MAP_IMAGE_ENTITY_LAYER_TYPE_ALL_EXCEPT_SWITCH)); + Graphics()->TextureSet(m_pImages->GetEntities(MAP_IMAGE_ENTITY_LAYER_TYPE_ALL_EXCEPT_SWITCH)); + } + else if(pTMap->m_Image >= 0 && pTMap->m_Image < m_pImages->Num()) + { + Graphics()->TextureSet(m_pImages->Get(pTMap->m_Image)); } else - Graphics()->TextureSet(m_pImages->Get(pTMap->m_Image)); + { + Graphics()->TextureClear(); + } CTile *pTiles = (CTile *)m_pLayers->Map()->GetData(pTMap->m_Data); unsigned int Size = m_pLayers->Map()->GetDataSize(pTMap->m_Data); @@ -1608,10 +1604,14 @@ void CMapLayers::OnRender() else if(pLayer->m_Type == LAYERTYPE_QUADS) { CMapItemLayerQuads *pQLayer = (CMapItemLayerQuads *)pLayer; - if(pQLayer->m_Image < 0 || pQLayer->m_Image >= m_pImages->Num()) - Graphics()->TextureClear(); - else + if(pQLayer->m_Image >= 0 && pQLayer->m_Image < m_pImages->Num()) + { Graphics()->TextureSet(m_pImages->Get(pQLayer->m_Image)); + } + else + { + Graphics()->TextureClear(); + } CQuad *pQuads = (CQuad *)m_pLayers->Map()->GetDataSwapped(pQLayer->m_Data); if(m_Type == TYPE_BACKGROUND_FORCE || m_Type == TYPE_FULL_DESIGN)