From f7722eb0164156f03e53ff97f6af6229f96c2479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Sat, 14 Oct 2023 11:40:18 +0200 Subject: [PATCH 1/5] Remove separate `CDataFileWriter::Init/OpenFile` functions Simplify usage of datafile writer by removing duplicate functions for opening file. --- src/engine/shared/datafile.cpp | 42 +++++++++++++-------------------- src/engine/shared/datafile.h | 4 +--- src/game/server/gamecontext.cpp | 3 +-- src/tools/config_store.cpp | 3 +-- 4 files changed, 20 insertions(+), 32 deletions(-) diff --git a/src/engine/shared/datafile.cpp b/src/engine/shared/datafile.cpp index f0a972f0f00..d0dba610588 100644 --- a/src/engine/shared/datafile.cpp +++ b/src/engine/shared/datafile.cpp @@ -596,9 +596,24 @@ int CDataFileReader::MapSize() const CDataFileWriter::CDataFileWriter() { m_File = 0; + m_pItemTypes = static_cast(calloc(MAX_ITEM_TYPES, sizeof(CItemTypeInfo))); + m_NumItemTypes = 0; + mem_zero(m_pItemTypes, sizeof(CItemTypeInfo) * MAX_ITEM_TYPES); + for(int i = 0; i < MAX_ITEM_TYPES; i++) + { + m_pItemTypes[i].m_First = -1; + m_pItemTypes[i].m_Last = -1; + } + m_pItems = static_cast(calloc(MAX_ITEMS, sizeof(CItemInfo))); + m_NumItems = 0; + m_pDatas = static_cast(calloc(MAX_DATAS, sizeof(CDataInfo))); + m_NumDatas = 0; + + mem_zero(m_aExtendedItemTypes, sizeof(m_aExtendedItemTypes)); + m_NumExtendedItemTypes = 0; } CDataFileWriter::~CDataFileWriter() @@ -634,36 +649,13 @@ CDataFileWriter::~CDataFileWriter() } } -bool CDataFileWriter::OpenFile(class IStorage *pStorage, const char *pFilename, int StorageType) +bool CDataFileWriter::Open(class IStorage *pStorage, const char *pFilename, int StorageType) { - dbg_assert(!m_File, "a file already exists"); + dbg_assert(!m_File, "File already open"); m_File = pStorage->OpenFile(pFilename, IOFLAG_WRITE, StorageType); return m_File != 0; } -void CDataFileWriter::Init() -{ - dbg_assert(!m_File, "a file already exists"); - m_NumItems = 0; - m_NumDatas = 0; - m_NumItemTypes = 0; - m_NumExtendedItemTypes = 0; - mem_zero(m_pItemTypes, sizeof(CItemTypeInfo) * MAX_ITEM_TYPES); - mem_zero(m_aExtendedItemTypes, sizeof(m_aExtendedItemTypes)); - - for(int i = 0; i < MAX_ITEM_TYPES; i++) - { - m_pItemTypes[i].m_First = -1; - m_pItemTypes[i].m_Last = -1; - } -} - -bool CDataFileWriter::Open(class IStorage *pStorage, const char *pFilename, int StorageType) -{ - Init(); - return OpenFile(pStorage, pFilename, StorageType); -} - int CDataFileWriter::GetTypeFromIndex(int Index) const { return ITEMTYPE_EX - Index - 1; diff --git a/src/engine/shared/datafile.h b/src/engine/shared/datafile.h index e5649f17a9b..8fd95849e1a 100644 --- a/src/engine/shared/datafile.h +++ b/src/engine/shared/datafile.h @@ -132,13 +132,11 @@ class CDataFileWriter } ~CDataFileWriter(); - void Init(); - bool OpenFile(class IStorage *pStorage, const char *pFilename, int StorageType = IStorage::TYPE_SAVE); bool Open(class IStorage *pStorage, const char *pFilename, int StorageType = IStorage::TYPE_SAVE); + int AddItem(int Type, int ID, int Size, const void *pData); int AddData(int Size, const void *pData, int CompressionLevel = Z_DEFAULT_COMPRESSION); int AddDataSwapped(int Size, const void *pData); int AddDataString(const char *pStr); - int AddItem(int Type, int ID, int Size, const void *pData); void Finish(); }; diff --git a/src/game/server/gamecontext.cpp b/src/game/server/gamecontext.cpp index 9c102fec35b..9263180149b 100644 --- a/src/game/server/gamecontext.cpp +++ b/src/game/server/gamecontext.cpp @@ -3866,7 +3866,6 @@ void CGameContext::OnMapChange(char *pNewMapName, int MapNameSize) Reader.Open(Storage(), pNewMapName, IStorage::TYPE_ALL); CDataFileWriter Writer; - Writer.Init(); int SettingsIndex = Reader.NumData(); bool FoundInfo = false; @@ -3944,7 +3943,7 @@ void CGameContext::OnMapChange(char *pNewMapName, int MapNameSize) free(pSettings); Reader.Close(); char aTemp[IO_MAX_PATH_LENGTH]; - Writer.OpenFile(Storage(), IStorage::FormatTmpPath(aTemp, sizeof(aTemp), pNewMapName)); + Writer.Open(Storage(), IStorage::FormatTmpPath(aTemp, sizeof(aTemp), pNewMapName)); Writer.Finish(); str_copy(pNewMapName, aTemp, MapNameSize); diff --git a/src/tools/config_store.cpp b/src/tools/config_store.cpp index 9a6f2eb3c79..0c06d058938 100644 --- a/src/tools/config_store.cpp +++ b/src/tools/config_store.cpp @@ -45,7 +45,6 @@ void Process(IStorage *pStorage, const char *pMapName, const char *pConfigName) Reader.Open(pStorage, pMapName, IStorage::TYPE_ABSOLUTE); CDataFileWriter Writer; - Writer.Init(); int SettingsIndex = Reader.NumData(); bool FoundInfo = false; @@ -123,7 +122,7 @@ void Process(IStorage *pStorage, const char *pMapName, const char *pConfigName) free(pSettings); Reader.Close(); - if(!Writer.OpenFile(pStorage, pMapName)) + if(!Writer.Open(pStorage, pMapName)) { dbg_msg("config_store", "couldn't open map file '%s' for writing", pMapName); return; From 284390cc752a5c8ae84459d48321777915d88b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 1 Nov 2023 12:17:02 +0100 Subject: [PATCH 2/5] Add assertions to ensure correct map item/data sizes are written Add assertions to prevent map items and data that are too large to be represented by the map format from being written to maps. Additionally, ensure that the size of the whole map file is not too large to be represented by the map header. Prevent `malloc` of size 0, which is implementation defined and should be avoided, when adding items without data, which happens for example when adding an empty array of envelope points. --- src/engine/shared/datafile.cpp | 56 +++++++++++++++++++++++----------- src/engine/shared/datafile.h | 6 ++-- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/engine/shared/datafile.cpp b/src/engine/shared/datafile.cpp index d0dba610588..3486efee6df 100644 --- a/src/engine/shared/datafile.cpp +++ b/src/engine/shared/datafile.cpp @@ -12,6 +12,7 @@ #include "uuid_manager.h" #include +#include static const int DEBUG = 0; @@ -681,11 +682,13 @@ int CDataFileWriter::GetExtendedItemTypeIndex(int Type) return Index; } -int CDataFileWriter::AddItem(int Type, int ID, int Size, const void *pData) +int CDataFileWriter::AddItem(int Type, int ID, size_t Size, const void *pData) { dbg_assert((Type >= 0 && Type < MAX_ITEM_TYPES) || Type >= OFFSET_UUID, "incorrect type"); dbg_assert(m_NumItems < 1024, "too many items"); - dbg_assert(Size % sizeof(int) == 0, "incorrect boundary"); + dbg_assert(Size == 0 || pData != nullptr, "Data missing"); // Items without data are allowed + dbg_assert(Size <= (size_t)std::numeric_limits::max(), "Data too large"); + dbg_assert(Size % sizeof(int) == 0, "Invalid data boundary"); if(Type >= OFFSET_UUID) { @@ -697,8 +700,13 @@ int CDataFileWriter::AddItem(int Type, int ID, int Size, const void *pData) m_pItems[m_NumItems].m_Size = Size; // copy data - m_pItems[m_NumItems].m_pData = malloc(Size); - mem_copy(m_pItems[m_NumItems].m_pData, pData, Size); + if(Size > 0) + { + m_pItems[m_NumItems].m_pData = malloc(Size); + mem_copy(m_pItems[m_NumItems].m_pData, pData, Size); + } + else + m_pItems[m_NumItems].m_pData = nullptr; if(!m_pItemTypes[Type].m_Num) // count item types m_NumItemTypes++; @@ -720,9 +728,11 @@ int CDataFileWriter::AddItem(int Type, int ID, int Size, const void *pData) return m_NumItems - 1; } -int CDataFileWriter::AddData(int Size, const void *pData, int CompressionLevel) +int CDataFileWriter::AddData(size_t Size, const void *pData, int CompressionLevel) { dbg_assert(m_NumDatas < 1024, "too much data"); + dbg_assert(Size > 0 && pData != nullptr, "Data missing"); + dbg_assert(Size <= (size_t)std::numeric_limits::max(), "Data too large"); CDataInfo *pInfo = &m_pDatas[m_NumDatas]; pInfo->m_pUncompressedData = malloc(Size); @@ -736,9 +746,11 @@ int CDataFileWriter::AddData(int Size, const void *pData, int CompressionLevel) return m_NumDatas - 1; } -int CDataFileWriter::AddDataSwapped(int Size, const void *pData) +int CDataFileWriter::AddDataSwapped(size_t Size, const void *pData) { - dbg_assert(Size % sizeof(int) == 0, "incorrect boundary"); + dbg_assert(Size > 0 && pData != nullptr, "Data missing"); + dbg_assert(Size <= (size_t)std::numeric_limits::max(), "Data too large"); + dbg_assert(Size % sizeof(int) == 0, "Invalid data boundary"); #if defined(CONF_ARCH_ENDIAN_BIG) void *pSwapped = malloc(Size); // temporary buffer that we use during compression @@ -754,6 +766,8 @@ int CDataFileWriter::AddDataSwapped(int Size, const void *pData) int CDataFileWriter::AddDataString(const char *pStr) { + dbg_assert(pStr != nullptr, "Data missing"); + if(pStr[0] == '\0') return -1; return AddData(str_length(pStr) + 1, pStr); @@ -785,27 +799,31 @@ void CDataFileWriter::Finish() } // calculate sizes - int ItemSize = 0; + size_t ItemSize = 0; for(int i = 0; i < m_NumItems; i++) { if(DEBUG) dbg_msg("datafile", "item=%d size=%d (%d)", i, m_pItems[i].m_Size, m_pItems[i].m_Size + (int)sizeof(CDatafileItem)); - ItemSize += m_pItems[i].m_Size + sizeof(CDatafileItem); + ItemSize += m_pItems[i].m_Size; + ItemSize += sizeof(CDatafileItem); } - int DataSize = 0; + size_t DataSize = 0; for(int i = 0; i < m_NumDatas; i++) DataSize += m_pDatas[i].m_CompressedSize; // calculate the complete size - const int TypesSize = m_NumItemTypes * sizeof(CDatafileItemType); - const int HeaderSize = sizeof(CDatafileHeader); - const int OffsetSize = (m_NumItems + m_NumDatas + m_NumDatas) * sizeof(int); // ItemOffsets, DataOffsets, DataUncompressedSizes - const int FileSize = HeaderSize + TypesSize + OffsetSize + ItemSize + DataSize; - const int SwapSize = FileSize - DataSize; + const size_t TypesSize = m_NumItemTypes * sizeof(CDatafileItemType); + const size_t HeaderSize = sizeof(CDatafileHeader); + const size_t OffsetSize = ((size_t)m_NumItems + m_NumDatas + m_NumDatas) * sizeof(int); // ItemOffsets, DataOffsets, DataUncompressedSizes + const size_t SwapSize = HeaderSize + TypesSize + OffsetSize + ItemSize; + const size_t FileSize = SwapSize + DataSize; if(DEBUG) - dbg_msg("datafile", "num_m_aItemTypes=%d TypesSize=%d m_aItemsize=%d DataSize=%d", m_NumItemTypes, TypesSize, ItemSize, DataSize); + dbg_msg("datafile", "m_NumItemTypes=%d TypesSize=%" PRIzu " ItemSize=%" PRIzu " DataSize=%" PRIzu, m_NumItemTypes, TypesSize, ItemSize, DataSize); + + // This also ensures that SwapSize, ItemSize and DataSize are valid. + dbg_assert(FileSize <= (size_t)std::numeric_limits::max(), "File size too large"); // construct Header { @@ -918,10 +936,12 @@ void CDataFileWriter::Finish() #if defined(CONF_ARCH_ENDIAN_BIG) swap_endian(&Item, sizeof(int), sizeof(Item) / sizeof(int)); - swap_endian(m_pItems[k].m_pData, sizeof(int), m_pItems[k].m_Size / sizeof(int)); + if(m_pItems[k].m_pData != nullptr) + swap_endian(m_pItems[k].m_pData, sizeof(int), m_pItems[k].m_Size / sizeof(int)); #endif io_write(m_File, &Item, sizeof(Item)); - io_write(m_File, m_pItems[k].m_pData, m_pItems[k].m_Size); + if(m_pItems[k].m_pData != nullptr) + io_write(m_File, m_pItems[k].m_pData, m_pItems[k].m_Size); // next k = m_pItems[k].m_Next; diff --git a/src/engine/shared/datafile.h b/src/engine/shared/datafile.h index 8fd95849e1a..70ffeccce91 100644 --- a/src/engine/shared/datafile.h +++ b/src/engine/shared/datafile.h @@ -133,9 +133,9 @@ class CDataFileWriter ~CDataFileWriter(); bool Open(class IStorage *pStorage, const char *pFilename, int StorageType = IStorage::TYPE_SAVE); - int AddItem(int Type, int ID, int Size, const void *pData); - int AddData(int Size, const void *pData, int CompressionLevel = Z_DEFAULT_COMPRESSION); - int AddDataSwapped(int Size, const void *pData); + int AddItem(int Type, int ID, size_t Size, const void *pData); + int AddData(size_t Size, const void *pData, int CompressionLevel = Z_DEFAULT_COMPRESSION); + int AddDataSwapped(size_t Size, const void *pData); int AddDataString(const char *pStr); void Finish(); }; From 64716075f9334cac480a7c514edd09f6f6c30fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 1 Nov 2023 12:45:02 +0100 Subject: [PATCH 3/5] Add assertion to ensure correct item IDs are written --- src/engine/shared/datafile.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/engine/shared/datafile.cpp b/src/engine/shared/datafile.cpp index 3486efee6df..8196a837899 100644 --- a/src/engine/shared/datafile.cpp +++ b/src/engine/shared/datafile.cpp @@ -684,7 +684,8 @@ int CDataFileWriter::GetExtendedItemTypeIndex(int Type) int CDataFileWriter::AddItem(int Type, int ID, size_t Size, const void *pData) { - dbg_assert((Type >= 0 && Type < MAX_ITEM_TYPES) || Type >= OFFSET_UUID, "incorrect type"); + dbg_assert((Type >= 0 && Type < MAX_ITEM_TYPES) || Type >= OFFSET_UUID, "Invalid type"); + dbg_assert(ID >= 0 && ID <= ITEMTYPE_EX, "Invalid ID"); dbg_assert(m_NumItems < 1024, "too many items"); dbg_assert(Size == 0 || pData != nullptr, "Data missing"); // Items without data are allowed dbg_assert(Size <= (size_t)std::numeric_limits::max(), "Data too large"); From 7343ca224cd63e9e8711546627ab766cf78a493e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 1 Nov 2023 12:45:45 +0100 Subject: [PATCH 4/5] Add `CDatafileHeader::SizeOffset` to replace magic `16` --- src/engine/shared/datafile.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/engine/shared/datafile.cpp b/src/engine/shared/datafile.cpp index 8196a837899..15d7fd7fb9f 100644 --- a/src/engine/shared/datafile.cpp +++ b/src/engine/shared/datafile.cpp @@ -66,6 +66,12 @@ struct CDatafileHeader int m_NumRawData; int m_ItemSize; int m_DataSize; + + constexpr size_t SizeOffset() + { + // The size of these members is not included in m_Size and m_Swaplen + return sizeof(m_aID) + sizeof(m_Version) + sizeof(m_Size) + sizeof(m_Swaplen); + } }; struct CDatafileInfo @@ -591,7 +597,7 @@ int CDataFileReader::MapSize() const { if(!m_pDataFile) return 0; - return m_pDataFile->m_Header.m_Size + 16; + return m_pDataFile->m_Header.m_Size + m_pDataFile->m_Header.SizeOffset(); } CDataFileWriter::CDataFileWriter() @@ -834,8 +840,8 @@ void CDataFileWriter::Finish() Header.m_aID[2] = 'T'; Header.m_aID[3] = 'A'; Header.m_Version = 4; - Header.m_Size = FileSize - 16; - Header.m_Swaplen = SwapSize - 16; + Header.m_Size = FileSize - Header.SizeOffset(); + Header.m_Swaplen = SwapSize - Header.SizeOffset(); Header.m_NumItemTypes = m_NumItemTypes; Header.m_NumItems = m_NumItems; Header.m_NumRawData = m_NumDatas; From 9b8eb9d6fca2df187b432b4623207d0cdc12a625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 1 Nov 2023 13:06:17 +0100 Subject: [PATCH 5/5] Use `for`- instead of `while`-loop, improve comments --- src/engine/shared/datafile.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/engine/shared/datafile.cpp b/src/engine/shared/datafile.cpp index 15d7fd7fb9f..ca164b368a9 100644 --- a/src/engine/shared/datafile.cpp +++ b/src/engine/shared/datafile.cpp @@ -926,14 +926,13 @@ void CDataFileWriter::Finish() io_write(m_File, &UncompressedSize, sizeof(UncompressedSize)); } - // write m_pItems + // write items sorted by type for(int i = 0; i < MAX_ITEM_TYPES; i++) { if(m_pItemTypes[i].m_Num) { - // write all m_pItems in of this type - int k = m_pItemTypes[i].m_First; - while(k != -1) + // write all items of this type + for(int k = m_pItemTypes[i].m_First; k != -1; k = m_pItems[k].m_Next) { CDatafileItem Item; Item.m_TypeAndID = (i << 16) | m_pItems[k].m_ID; @@ -949,9 +948,6 @@ void CDataFileWriter::Finish() io_write(m_File, &Item, sizeof(Item)); if(m_pItems[k].m_pData != nullptr) io_write(m_File, m_pItems[k].m_pData, m_pItems[k].m_Size); - - // next - k = m_pItems[k].m_Next; } } }