mirror of
https://github.com/ddnet/ddnet.git
synced 2024-11-10 01:58:19 +00:00
Merge #5656
5656: Validate snapshot size and member variables and demo snapshots r=def- a=Robyt3 Add `CSnapshot::IsValid` to check if a snapshot unpacked from a snapshot delta or demo is valid: - ensure number of items and data size are not negative - ensure that the actual size of the snapshot matches the size derived from its member variables - ensure item offsets are within the valid range - ensure item sizes are not negative Add `CSnapshot::TotalSize` and `CSnapshot::OffsetSize` utility functions. Minor improvements to related error messages. Fixes buffer overflow (see #5646): ``` ==47744==ERROR: AddressSanitizer: global-buffer-overflow on address 0x558618e3767f at pc 0x558614b9bdfb bp 0x7ffe58a32cd0 sp 0x7ffe58a32cc0 READ of size 4 at 0x558618e3767f thread T0 0x558614b9bdfa in CSnapshotItem::Type() const src/engine/shared/snapshot.h:16 0x558615c3c911 in CSnapshot::GetItemType(int) const src/engine/shared/snapshot.cpp:29 0x558614aebaba in CClient::UnpackAndValidateSnapshot(CSnapshot*, CSnapshot*) src/engine/client/client.cpp:2264 0x558614af87cb in CClient::OnDemoPlayerSnapshot(void*, int) src/engine/client/client.cpp:2598 0x558615b9db1a in CDemoPlayer::DoTick() src/engine/shared/demo.cpp:659 0x558615babd3f in CDemoPlayer::Update(bool) src/engine/shared/demo.cpp:1007 0x558614afb08b in CClient::Update() src/engine/client/client.cpp:2686 0x558614b1d9eb in CClient::Run() src/engine/client/client.cpp:3296 0x558614b8e64f in main src/engine/client/client.cpp:4761 ``` And fixes a buffer overflow that manifests itself as an internal ASan error: ``` ================================================================= ==4755==AddressSanitizer CHECK failed: ../../../../src/libsanitizer/asan/asan_descriptions.cc:79 "((0 && "Address is not in memory and not in shadow?")) != (0)" (0x0, 0x0) 0x7f0bf5f368be in AsanCheckFailed ../../../../src/libsanitizer/asan/asan_rtl.cc:72 0x7f0bf5f54eee in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) ../../../../src/libsanitizer/sanitizer_common/sanitizer_termination.cc:77 0x7f0bf5e4cb6f in GetShadowKind ../../../../src/libsanitizer/asan/asan_descriptions.cc:79 0x7f0bf5e4cb6f in __asan::GetShadowAddressInformation(unsigned long, __asan::ShadowAddressDescription*) ../../../../src/libsanitizer/asan/asan_descriptions.cc:95 0x7f0bf5e4cb6f in __asan::GetShadowAddressInformation(unsigned long, __asan::ShadowAddressDescription*) ../../../../src/libsanitizer/asan/asan_descriptions.cc:92 0x7f0bf5e4e386 in __asan::AddressDescription::AddressDescription(unsigned long, unsigned long, bool) ../../../../src/libsanitizer/asan/asan_descriptions.cc:440 0x7f0bf5e50e94 in __asan::ErrorGeneric::ErrorGeneric(unsigned int, unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long) ../../../../src/libsanitizer/asan/asan_errors.cc:380 0x7f0bf5f35f4d in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) ../../../../src/libsanitizer/asan/asan_report.cc:460 0x7f0bf5e86f5e in __interceptor_memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:762 0x558234873f1d in mem_zero src/base/system.cpp:213 0x55823481fc27 in CSnapshotBuilder::NewItem(int, int, int) src/engine/shared/snapshot.cpp:675 0x55823481be65 in CSnapshotDelta::UnpackDelta(CSnapshot*, CSnapshot*, void const*, int) src/engine/shared/snapshot.cpp:380 0x558234776641 in CDemoPlayer::DoTick() src/engine/shared/demo.cpp:631 0x5582347861a9 in CDemoPlayer::Update(bool) src/engine/shared/demo.cpp:1007 0x5582336d4c7d in CClient::Update() src/engine/client/client.cpp:2695 0x5582336f75dd in CClient::Run() src/engine/client/client.cpp:3305 0x558233768241 in main src/engine/client/client.cpp:4770 ``` ## Checklist - [X] Tested the change ingame - [ ] Provided screenshots if it is a visual change - [ ] Tested in combination with possibly related configuration options - [ ] Written a unit test (especially base/) or added coverage to integration test - [X] Considered possible null pointers and out of bounds array indexing - [X] Changed no physics that affect existing maps - [X] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional) Co-authored-by: Robert Müller <robytemueller@gmail.com>
This commit is contained in:
bors[bot]
GitHub
commit
7f44cdeb55
|
|
@ -2053,7 +2053,12 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy)
|
|||
const int SnapSize = m_SnapshotDelta.UnpackDelta(pDeltaShot, pTmpBuffer3, pDeltaData, DeltaSize);
|
||||
if(SnapSize < 0)
|
||||
{
|
||||
dbg_msg("client", "delta unpack failed!=%d", SnapSize);
|
||||
dbg_msg("client", "delta unpack failed. error=%d", SnapSize);
|
||||
return;
|
||||
}
|
||||
if(!pTmpBuffer3->IsValid(SnapSize))
|
||||
{
|
||||
dbg_msg("client", "snapshot invalid. SnapSize=%d, DeltaSize=%d", SnapSize, DeltaSize);
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -2097,7 +2102,7 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy)
|
|||
const int AltSnapSize = UnpackAndValidateSnapshot(pTmpBuffer3, pAltSnapBuffer);
|
||||
if(AltSnapSize < 0)
|
||||
{
|
||||
dbg_msg("client", "unpack snapshot and validate failed!=%d", AltSnapSize);
|
||||
dbg_msg("client", "unpack snapshot and validate failed. error=%d", AltSnapSize);
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -2581,26 +2586,22 @@ void CClient::OnDemoPlayerSnapshot(void *pData, int Size)
|
|||
{
|
||||
// update ticks, they could have changed
|
||||
const CDemoPlayer::CPlaybackInfo *pInfo = m_DemoPlayer.Info();
|
||||
CSnapshotStorage::CHolder *pTemp;
|
||||
m_aCurGameTick[g_Config.m_ClDummy] = pInfo->m_Info.m_CurrentTick;
|
||||
m_aPrevGameTick[g_Config.m_ClDummy] = pInfo->m_PreviousTick;
|
||||
|
||||
// handle snapshots
|
||||
pTemp = m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV];
|
||||
m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV] = m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT];
|
||||
m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT] = pTemp;
|
||||
|
||||
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pSnap, pData, Size);
|
||||
|
||||
// create a verified and unpacked snapshot
|
||||
unsigned char aAltSnapBuffer[CSnapshot::MAX_SIZE];
|
||||
CSnapshot *pAltSnapBuffer = (CSnapshot *)aAltSnapBuffer;
|
||||
const int AltSnapSize = UnpackAndValidateSnapshot((CSnapshot *)pData, pAltSnapBuffer);
|
||||
if(AltSnapSize < 0)
|
||||
{
|
||||
dbg_msg("client", "unpack snapshot and validate failed!=%d", AltSnapSize);
|
||||
dbg_msg("client", "unpack snapshot and validate failed. error=%d", AltSnapSize);
|
||||
return;
|
||||
}
|
||||
|
||||
// handle snapshots after validation
|
||||
std::swap(m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV], m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]);
|
||||
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pSnap, pData, Size);
|
||||
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pAltSnap, pAltSnapBuffer, AltSnapSize);
|
||||
|
||||
GameClient()->OnNewSnapshot();
|
||||
|
|
|
|||
|
|
@ -534,17 +534,10 @@ void CDemoPlayer::ScanFile()
|
|||
|
||||
void CDemoPlayer::DoTick()
|
||||
{
|
||||
static char s_aCompresseddata[CSnapshot::MAX_SIZE];
|
||||
static char s_aDecompressed[CSnapshot::MAX_SIZE];
|
||||
static char s_aData[CSnapshot::MAX_SIZE];
|
||||
int ChunkType, ChunkTick, ChunkSize;
|
||||
int DataSize = 0;
|
||||
int GotSnapshot = 0;
|
||||
|
||||
// update ticks
|
||||
m_Info.m_PreviousTick = m_Info.m_Info.m_CurrentTick;
|
||||
m_Info.m_Info.m_CurrentTick = m_Info.m_NextTick;
|
||||
ChunkTick = m_Info.m_Info.m_CurrentTick;
|
||||
int ChunkTick = m_Info.m_Info.m_CurrentTick;
|
||||
|
||||
int64_t Freq = time_freq();
|
||||
int64_t CurtickStart = (m_Info.m_Info.m_CurrentTick) * Freq / SERVER_TICK_SPEED;
|
||||
|
|
@ -555,8 +548,10 @@ void CDemoPlayer::DoTick()
|
|||
if(m_UpdateIntraTimesFunc)
|
||||
m_UpdateIntraTimesFunc();
|
||||
|
||||
bool GotSnapshot = false;
|
||||
while(true)
|
||||
{
|
||||
int ChunkType, ChunkSize;
|
||||
if(ReadChunkHeader(&ChunkType, &ChunkSize, &ChunkTick))
|
||||
{
|
||||
// stop on error or eof
|
||||
|
|
@ -578,8 +573,11 @@ void CDemoPlayer::DoTick()
|
|||
}
|
||||
|
||||
// read the chunk
|
||||
int DataSize = 0;
|
||||
static char s_aData[CSnapshot::MAX_SIZE];
|
||||
if(ChunkSize)
|
||||
{
|
||||
static char s_aCompresseddata[CSnapshot::MAX_SIZE];
|
||||
if(io_read(m_File, s_aCompresseddata, ChunkSize) != (unsigned)ChunkSize)
|
||||
{
|
||||
// stop on error or eof
|
||||
|
|
@ -589,6 +587,7 @@ void CDemoPlayer::DoTick()
|
|||
break;
|
||||
}
|
||||
|
||||
static char s_aDecompressed[CSnapshot::MAX_SIZE];
|
||||
DataSize = CNetBase::Decompress(s_aCompresseddata, ChunkSize, s_aDecompressed, sizeof(s_aDecompressed));
|
||||
if(DataSize < 0)
|
||||
{
|
||||
|
|
@ -614,20 +613,10 @@ void CDemoPlayer::DoTick()
|
|||
{
|
||||
// process delta snapshot
|
||||
static char s_aNewsnap[CSnapshot::MAX_SIZE];
|
||||
CSnapshot *pNewsnap = (CSnapshot *)s_aNewsnap;
|
||||
DataSize = m_pSnapshotDelta->UnpackDelta((CSnapshot *)m_aLastSnapshotData, pNewsnap, s_aData, DataSize);
|
||||
|
||||
GotSnapshot = 1;
|
||||
|
||||
DataSize = m_pSnapshotDelta->UnpackDelta((CSnapshot *)m_aLastSnapshotData, (CSnapshot *)s_aNewsnap, s_aData, DataSize);
|
||||
|
||||
if(DataSize >= 0)
|
||||
{
|
||||
if(m_pListener)
|
||||
m_pListener->OnDemoPlayerSnapshot(s_aNewsnap, DataSize);
|
||||
|
||||
m_LastSnapshotDataSize = DataSize;
|
||||
mem_copy(m_aLastSnapshotData, s_aNewsnap, DataSize);
|
||||
}
|
||||
else
|
||||
if(DataSize < 0)
|
||||
{
|
||||
if(m_pConsole)
|
||||
{
|
||||
|
|
@ -636,23 +625,54 @@ void CDemoPlayer::DoTick()
|
|||
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
|
||||
}
|
||||
}
|
||||
else if(!pNewsnap->IsValid(DataSize))
|
||||
{
|
||||
if(m_pConsole)
|
||||
{
|
||||
char aBuf[256];
|
||||
str_format(aBuf, sizeof(aBuf), "snapshot delta invalid. DataSize=%d", DataSize);
|
||||
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
if(m_pListener)
|
||||
m_pListener->OnDemoPlayerSnapshot(s_aNewsnap, DataSize);
|
||||
|
||||
m_LastSnapshotDataSize = DataSize;
|
||||
mem_copy(m_aLastSnapshotData, s_aNewsnap, DataSize);
|
||||
GotSnapshot = true;
|
||||
}
|
||||
}
|
||||
else if(ChunkType == CHUNKTYPE_SNAPSHOT)
|
||||
{
|
||||
// process full snapshot
|
||||
GotSnapshot = 1;
|
||||
CSnapshot *pSnap = (CSnapshot *)s_aData;
|
||||
if(!pSnap->IsValid(DataSize))
|
||||
{
|
||||
if(m_pConsole)
|
||||
{
|
||||
char aBuf[256];
|
||||
str_format(aBuf, sizeof(aBuf), "snapshot invalid. DataSize=%d", DataSize);
|
||||
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
GotSnapshot = true;
|
||||
|
||||
m_LastSnapshotDataSize = DataSize;
|
||||
mem_copy(m_aLastSnapshotData, s_aData, DataSize);
|
||||
if(m_pListener)
|
||||
m_pListener->OnDemoPlayerSnapshot(s_aData, DataSize);
|
||||
m_LastSnapshotDataSize = DataSize;
|
||||
mem_copy(m_aLastSnapshotData, s_aData, DataSize);
|
||||
if(m_pListener)
|
||||
m_pListener->OnDemoPlayerSnapshot(s_aData, DataSize);
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
// if there were no snapshots in this tick, replay the last one
|
||||
if(!GotSnapshot && m_pListener && m_LastSnapshotDataSize != -1)
|
||||
{
|
||||
GotSnapshot = 1;
|
||||
GotSnapshot = true;
|
||||
m_pListener->OnDemoPlayerSnapshot(m_aLastSnapshotData, m_LastSnapshotDataSize);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -122,6 +122,26 @@ void CSnapshot::DebugDump()
|
|||
}
|
||||
}
|
||||
|
||||
bool CSnapshot::IsValid(size_t ActualSize) const
|
||||
{
|
||||
// validate total size
|
||||
if(ActualSize < sizeof(CSnapshot) || m_NumItems < 0 || m_DataSize < 0 || ActualSize != TotalSize())
|
||||
return false;
|
||||
|
||||
// validate item offsets
|
||||
const int *pOffsets = Offsets();
|
||||
for(int Index = 0; Index < m_NumItems; Index++)
|
||||
if(pOffsets[Index] < 0 || pOffsets[Index] > m_DataSize)
|
||||
return false;
|
||||
|
||||
// validate item sizes
|
||||
for(int Index = 0; Index < m_NumItems; Index++)
|
||||
if(GetItemSize(Index) < 0) // the offsets must be validated before using this
|
||||
return false;
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
// CSnapshotDelta
|
||||
|
||||
struct CItemList
|
||||
|
|
@ -574,14 +594,13 @@ int *CSnapshotBuilder::GetItemData(int Key)
|
|||
|
||||
int CSnapshotBuilder::Finish(void *pSnapData)
|
||||
{
|
||||
// flattern and make the snapshot
|
||||
// flatten and make the snapshot
|
||||
CSnapshot *pSnap = (CSnapshot *)pSnapData;
|
||||
int OffsetSize = sizeof(int) * m_NumItems;
|
||||
pSnap->m_DataSize = m_DataSize;
|
||||
pSnap->m_NumItems = m_NumItems;
|
||||
mem_copy(pSnap->Offsets(), m_aOffsets, OffsetSize);
|
||||
mem_copy(pSnap->Offsets(), m_aOffsets, pSnap->OffsetSize());
|
||||
mem_copy(pSnap->DataStart(), m_aData, m_DataSize);
|
||||
return sizeof(CSnapshot) + OffsetSize + m_DataSize;
|
||||
return pSnap->TotalSize();
|
||||
}
|
||||
|
||||
int CSnapshotBuilder::GetTypeFromIndex(int Index)
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
#ifndef ENGINE_SHARED_SNAPSHOT_H
|
||||
#define ENGINE_SHARED_SNAPSHOT_H
|
||||
|
||||
#include <cstddef>
|
||||
#include <stdint.h>
|
||||
|
||||
// CSnapshot
|
||||
|
|
@ -27,6 +28,9 @@ class CSnapshot
|
|||
int *Offsets() const { return (int *)(this + 1); }
|
||||
char *DataStart() const { return (char *)(Offsets() + m_NumItems); }
|
||||
|
||||
size_t OffsetSize() const { return sizeof(int) * m_NumItems; }
|
||||
size_t TotalSize() const { return sizeof(CSnapshot) + OffsetSize() + m_DataSize; }
|
||||
|
||||
public:
|
||||
enum
|
||||
{
|
||||
|
|
@ -53,6 +57,7 @@ public:
|
|||
|
||||
unsigned Crc();
|
||||
void DebugDump();
|
||||
bool IsValid(size_t ActualSize) const;
|
||||
};
|
||||
|
||||
// CSnapshotDelta
|
||||
|
|
|
|||
Loading…
Reference in a new issue