From 67068ade9085dedc73a45726cb6fa7aa6cae23c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 18 Sep 2024 23:58:42 +0200 Subject: [PATCH 1/5] Fix snapshot builder creating too large snapshots The snapshot builder was only considering the size of the data and of the new item being created in the `NewItem` function but not the size of the `CSnapshot` class and the size of the offsets (one `int` for each item including the new one). This could lead to snapshots being too large, which could cause the server to crash when the snapshots were copied into buffers of size `CSnapshot::MAX_SIZE`. However, it should be unlikely for this to happen normally, as the maximum number of snap items (`CSnapshot::MAX_ITEMS`, which is `1024`) is usually reached before the maximum snapshot size. Also check in the `CSnapshot::IsValid` function that the snapshot is not too large (`CSnapshot::MAX_SIZE`) and check that the number of items is not too large (`CSnapshot::MAX_ITEMS`). --- src/engine/shared/snapshot.cpp | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/engine/shared/snapshot.cpp b/src/engine/shared/snapshot.cpp index 76e920d6a..0e2b777f8 100644 --- a/src/engine/shared/snapshot.cpp +++ b/src/engine/shared/snapshot.cpp @@ -135,8 +135,15 @@ void CSnapshot::DebugDump() const bool CSnapshot::IsValid(size_t ActualSize) const { // validate total size - if(ActualSize < sizeof(CSnapshot) || m_NumItems < 0 || m_DataSize < 0 || ActualSize != TotalSize()) + if(ActualSize < sizeof(CSnapshot) || + ActualSize > MAX_SIZE || + m_NumItems < 0 || + m_NumItems > MAX_ITEMS || + m_DataSize < 0 || + ActualSize != TotalSize()) + { return false; + } // validate item offsets const int *pOffsets = Offsets(); @@ -744,12 +751,15 @@ int *CSnapshotBuilder::GetItemData(int Key) int CSnapshotBuilder::Finish(void *pSnapData) { // flatten and make the snapshot + dbg_assert(m_NumItems <= CSnapshot::MAX_ITEMS, "Too many snap items"); CSnapshot *pSnap = (CSnapshot *)pSnapData; pSnap->m_DataSize = m_DataSize; pSnap->m_NumItems = m_NumItems; + const size_t TotalSize = pSnap->TotalSize(); + dbg_assert(TotalSize <= (size_t)CSnapshot::MAX_SIZE, "Snapshot too large"); mem_copy(pSnap->Offsets(), m_aOffsets, pSnap->OffsetSize()); mem_copy(pSnap->DataStart(), m_aData, m_DataSize); - return pSnap->TotalSize(); + return TotalSize; } int CSnapshotBuilder::GetTypeFromIndex(int Index) const @@ -794,11 +804,15 @@ void *CSnapshotBuilder::NewItem(int Type, int Id, int Size) return nullptr; } - if(m_DataSize + sizeof(CSnapshotItem) + Size >= CSnapshot::MAX_SIZE || - m_NumItems + 1 >= CSnapshot::MAX_ITEMS) + if(m_NumItems >= CSnapshot::MAX_ITEMS) + { + return nullptr; + } + + const size_t OffsetSize = (m_NumItems + 1) * sizeof(int); + const size_t ItemSize = sizeof(CSnapshotItem) + Size; + if(sizeof(CSnapshot) + OffsetSize + m_DataSize + ItemSize > CSnapshot::MAX_SIZE) { - dbg_assert(m_DataSize < CSnapshot::MAX_SIZE, "too much data"); - dbg_assert(m_NumItems < CSnapshot::MAX_ITEMS, "too many items"); return nullptr; } @@ -824,11 +838,11 @@ void *CSnapshotBuilder::NewItem(int Type, int Id, int Size) else if(Type < 0) return nullptr; - mem_zero(pObj, sizeof(CSnapshotItem) + Size); pObj->m_TypeAndId = (Type << 16) | Id; m_aOffsets[m_NumItems] = m_DataSize; - m_DataSize += sizeof(CSnapshotItem) + Size; + m_DataSize += ItemSize; m_NumItems++; + mem_zero(pObj->Data(), Size); return pObj->Data(); } From 9a380ff199eafc35f59d8b231cc7e2757b62ff12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 19 Sep 2024 14:09:05 +0200 Subject: [PATCH 2/5] Do not add snap item if extended item type could not be added The `CSnapshotBuilder::AddExtendedItemType` function can fail if the snapshot is already full. Previously, this was not handled, leading to `m_aExtendedItemTypes` being updated inconsistently with the real extended item types and the snap item being added without the extended item type. Now, the snap item is not added if its extended item type could not be added. --- src/engine/shared/snapshot.cpp | 39 ++++++++++++++++++++++------------ src/engine/shared/snapshot.h | 2 +- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/engine/shared/snapshot.cpp b/src/engine/shared/snapshot.cpp index 0e2b777f8..5d1222357 100644 --- a/src/engine/shared/snapshot.cpp +++ b/src/engine/shared/snapshot.cpp @@ -767,17 +767,22 @@ int CSnapshotBuilder::GetTypeFromIndex(int Index) const return CSnapshot::MAX_TYPE - Index; } -void CSnapshotBuilder::AddExtendedItemType(int Index) +bool CSnapshotBuilder::AddExtendedItemType(int Index) { dbg_assert(0 <= Index && Index < m_NumExtendedItemTypes, "index out of range"); - int TypeId = m_aExtendedItemTypes[Index]; - CUuid Uuid = g_UuidManager.GetUuid(TypeId); - int *pUuidItem = (int *)NewItem(0, GetTypeFromIndex(Index), sizeof(Uuid)); // NETOBJTYPE_EX - if(pUuidItem) + int *pUuidItem = static_cast(NewItem(0, GetTypeFromIndex(Index), sizeof(CUuid))); // NETOBJTYPE_EX + if(pUuidItem == nullptr) { - for(size_t i = 0; i < sizeof(CUuid) / sizeof(int32_t); i++) - pUuidItem[i] = bytes_be_to_uint(&Uuid.m_aData[i * sizeof(int32_t)]); + return false; } + + const int TypeId = m_aExtendedItemTypes[Index]; + const CUuid Uuid = g_UuidManager.GetUuid(TypeId); + for(size_t i = 0; i < sizeof(CUuid) / sizeof(int32_t); i++) + { + pUuidItem[i] = bytes_be_to_uint(&Uuid.m_aData[i * sizeof(int32_t)]); + } + return true; } int CSnapshotBuilder::GetExtendedItemTypeIndex(int TypeId) @@ -793,8 +798,12 @@ int CSnapshotBuilder::GetExtendedItemTypeIndex(int TypeId) int Index = m_NumExtendedItemTypes; m_NumExtendedItemTypes++; m_aExtendedItemTypes[Index] = TypeId; - AddExtendedItemType(Index); - return Index; + if(AddExtendedItemType(Index)) + { + return Index; + } + m_NumExtendedItemTypes--; + return -1; } void *CSnapshotBuilder::NewItem(int Type, int Id, int Size) @@ -816,11 +825,15 @@ void *CSnapshotBuilder::NewItem(int Type, int Id, int Size) return nullptr; } - bool Extended = false; - if(Type >= OFFSET_UUID) + const bool Extended = Type >= OFFSET_UUID; + if(Extended) { - Extended = true; - Type = GetTypeFromIndex(GetExtendedItemTypeIndex(Type)); + const int ExtendedItemTypeIndex = GetExtendedItemTypeIndex(Type); + if(ExtendedItemTypeIndex == -1) + { + return nullptr; + } + Type = GetTypeFromIndex(ExtendedItemTypeIndex); } CSnapshotItem *pObj = (CSnapshotItem *)(m_aData + m_DataSize); diff --git a/src/engine/shared/snapshot.h b/src/engine/shared/snapshot.h index 24aeaafa9..7a001221d 100644 --- a/src/engine/shared/snapshot.h +++ b/src/engine/shared/snapshot.h @@ -158,7 +158,7 @@ class CSnapshotBuilder int m_aExtendedItemTypes[MAX_EXTENDED_ITEM_TYPES]; int m_NumExtendedItemTypes; - void AddExtendedItemType(int Index); + bool AddExtendedItemType(int Index); int GetExtendedItemTypeIndex(int TypeId); int GetTypeFromIndex(int Index) const; From ac9c66047c1dd9b959355018820bec75da1eff0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 19 Sep 2024 13:38:02 +0200 Subject: [PATCH 3/5] Extract `pItem` variable to avoid duplicate `GetItem` call --- src/engine/shared/snapshot.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/engine/shared/snapshot.cpp b/src/engine/shared/snapshot.cpp index 5d1222357..99d2b50cc 100644 --- a/src/engine/shared/snapshot.cpp +++ b/src/engine/shared/snapshot.cpp @@ -742,8 +742,11 @@ int *CSnapshotBuilder::GetItemData(int Key) { for(int i = 0; i < m_NumItems; i++) { - if(GetItem(i)->Key() == Key) - return GetItem(i)->Data(); + CSnapshotItem *pItem = GetItem(i); + if(pItem->Key() == Key) + { + return pItem->Data(); + } } return nullptr; } From 2426d59ab5302841d277a5a997357296c95eb52b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 19 Sep 2024 13:38:11 +0200 Subject: [PATCH 4/5] Remove unnecessary checks in `CSnapshotBuilder::Init7` function The client now validates snapshots including their maximum size, so these additional checks for recording snapshots to demos are unnecessary. --- src/engine/shared/sixup_translate_snapshot.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/engine/shared/sixup_translate_snapshot.cpp b/src/engine/shared/sixup_translate_snapshot.cpp index f693b45c8..2b06e5369 100644 --- a/src/engine/shared/sixup_translate_snapshot.cpp +++ b/src/engine/shared/sixup_translate_snapshot.cpp @@ -8,17 +8,6 @@ void CSnapshotBuilder::Init7(const CSnapshot *pSnapshot) // but the snap we are building is a 0.6 snap m_Sixup = false; - if(pSnapshot->m_DataSize + sizeof(CSnapshot) + pSnapshot->m_NumItems * sizeof(int) * 2 > CSnapshot::MAX_SIZE || pSnapshot->m_NumItems > CSnapshot::MAX_ITEMS) - { - // key and offset per item - dbg_assert(m_DataSize + sizeof(CSnapshot) + m_NumItems * sizeof(int) * 2 < CSnapshot::MAX_SIZE, "too much data"); - dbg_assert(m_NumItems < CSnapshot::MAX_ITEMS, "too many items"); - dbg_msg("sixup", "demo recording failed on invalid snapshot"); - m_DataSize = 0; - m_NumItems = 0; - return; - } - m_DataSize = pSnapshot->m_DataSize; m_NumItems = pSnapshot->m_NumItems; mem_copy(m_aOffsets, pSnapshot->Offsets(), sizeof(int) * m_NumItems); From fd01e6c8056effe774bcda9db8cf733824a5b4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 19 Sep 2024 14:00:44 +0200 Subject: [PATCH 5/5] Fix snapshot handling when converting 0.7 demo snapshot fails Only prevent demo recording of the snapshot instead of ignoring most of the snapshot by returning early when converting the snapshot for demo recording fails. --- src/engine/client/client.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/engine/client/client.cpp b/src/engine/client/client.cpp index 5f05e5d5d..731227b15 100644 --- a/src/engine/client/client.cpp +++ b/src/engine/client/client.cpp @@ -2006,17 +2006,19 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy) if(DemoSnapSize < 0) { dbg_msg("sixup", "demo snapshot failed. error=%d", DemoSnapSize); - return; } } - // add snapshot to demo - for(auto &DemoRecorder : m_aDemoRecorder) + if(DemoSnapSize >= 0) { - if(DemoRecorder.IsRecording()) + // add snapshot to demo + for(auto &DemoRecorder : m_aDemoRecorder) { - // write snapshot - DemoRecorder.RecordSnapshot(GameTick, IsSixup() ? pSnapSeven : pTmpBuffer3, DemoSnapSize); + if(DemoRecorder.IsRecording()) + { + // write snapshot + DemoRecorder.RecordSnapshot(GameTick, IsSixup() ? pSnapSeven : pTmpBuffer3, DemoSnapSize); + } } } }