From ef48a2fde4f03884d019db6106d9189b1ac22498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 3 May 2023 17:59:36 +0200 Subject: [PATCH 1/5] Use `sizeof(int)` instead of `4` --- src/engine/shared/packer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/shared/packer.cpp b/src/engine/shared/packer.cpp index 77565bdd3..21de58aa8 100644 --- a/src/engine/shared/packer.cpp +++ b/src/engine/shared/packer.cpp @@ -128,14 +128,14 @@ int CUnpacker::GetUncompressedInt() if(m_Error) return 0; - if(m_pCurrent + 4 > m_pEnd) + if(m_pCurrent + sizeof(int) > m_pEnd) { m_Error = 1; return 0; } int i = *(int *)m_pCurrent; - m_pCurrent += 4; + m_pCurrent += sizeof(int); return i; } From fd4491b307399ccbdf6e089519fb30a429ce50fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 3 May 2023 18:00:42 +0200 Subject: [PATCH 2/5] Use `bool` instead of `int` --- src/engine/shared/packer.cpp | 22 +++++++++++----------- src/engine/shared/packer.h | 4 ++-- src/test/packer.cpp | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/engine/shared/packer.cpp b/src/engine/shared/packer.cpp index 21de58aa8..a8a4de6a6 100644 --- a/src/engine/shared/packer.cpp +++ b/src/engine/shared/packer.cpp @@ -7,7 +7,7 @@ void CPacker::Reset() { - m_Error = 0; + m_Error = false; m_pCurrent = m_aBuffer; m_pEnd = m_pCurrent + PACKER_BUFFER_SIZE; } @@ -20,7 +20,7 @@ void CPacker::AddInt(int i) unsigned char *pNext = CVariableInt::Pack(m_pCurrent, i, m_pEnd - m_pCurrent); if(!pNext) { - m_Error = 1; + m_Error = true; return; } m_pCurrent = pNext; @@ -51,7 +51,7 @@ void CPacker::AddString(const char *pStr, int Limit) // Ensure space for the null termination. if(m_pEnd - m_pCurrent < Length + 1) { - m_Error = 1; + m_Error = true; break; } Length = str_utf8_encode((char *)m_pCurrent, Codepoint); @@ -68,7 +68,7 @@ void CPacker::AddRaw(const void *pData, int Size) if(m_pCurrent + Size >= m_pEnd) { - m_Error = 1; + m_Error = true; return; } @@ -82,7 +82,7 @@ void CPacker::AddRaw(const void *pData, int Size) void CUnpacker::Reset(const void *pData, int Size) { - m_Error = 0; + m_Error = false; m_pStart = (const unsigned char *)pData; m_pEnd = m_pStart + Size; m_pCurrent = m_pStart; @@ -95,7 +95,7 @@ int CUnpacker::GetInt() if(m_pCurrent >= m_pEnd) { - m_Error = 1; + m_Error = true; return 0; } @@ -103,7 +103,7 @@ int CUnpacker::GetInt() const unsigned char *pNext = CVariableInt::Unpack(m_pCurrent, &i, m_pEnd - m_pCurrent); if(!pNext) { - m_Error = 1; + m_Error = true; return 0; } m_pCurrent = pNext; @@ -130,7 +130,7 @@ int CUnpacker::GetUncompressedInt() if(m_pCurrent + sizeof(int) > m_pEnd) { - m_Error = 1; + m_Error = true; return 0; } @@ -159,7 +159,7 @@ const char *CUnpacker::GetString(int SanitizeType) if(m_pCurrent >= m_pEnd) { - m_Error = 1; + m_Error = true; return ""; } @@ -169,7 +169,7 @@ const char *CUnpacker::GetString(int SanitizeType) m_pCurrent++; if(m_pCurrent == m_pEnd) { - m_Error = 1; + m_Error = true; return ""; } } @@ -192,7 +192,7 @@ const unsigned char *CUnpacker::GetRaw(int Size) // check for nasty sizes if(Size < 0 || m_pCurrent + Size > m_pEnd) { - m_Error = 1; + m_Error = true; return 0; } diff --git a/src/engine/shared/packer.h b/src/engine/shared/packer.h index da0c2c938..2c428a1e3 100644 --- a/src/engine/shared/packer.h +++ b/src/engine/shared/packer.h @@ -15,7 +15,7 @@ private: unsigned char m_aBuffer[PACKER_BUFFER_SIZE]; unsigned char *m_pCurrent; unsigned char *m_pEnd; - int m_Error; + bool m_Error; public: void Reset(); @@ -33,7 +33,7 @@ class CUnpacker const unsigned char *m_pStart; const unsigned char *m_pCurrent; const unsigned char *m_pEnd; - int m_Error; + bool m_Error; public: enum diff --git a/src/test/packer.cpp b/src/test/packer.cpp index cfe465c4b..90126c365 100644 --- a/src/test/packer.cpp +++ b/src/test/packer.cpp @@ -32,7 +32,7 @@ static void ExpectAddInt(int Input, int Expected) CPacker Packer; Packer.Reset(); Packer.AddInt(Input); - EXPECT_EQ(Packer.Error(), 0); + EXPECT_EQ(Packer.Error(), false); ASSERT_EQ(Packer.Size(), 1); EXPECT_EQ(Packer.Data()[0], Expected); } @@ -42,7 +42,7 @@ static void ExpectAddExtendedInt(int Input, unsigned char *pExpected, int Size) CPacker Packer; Packer.Reset(); Packer.AddInt(Input); - EXPECT_EQ(Packer.Error(), 0); + EXPECT_EQ(Packer.Error(), false); ASSERT_EQ(Packer.Size(), Size); EXPECT_EQ(mem_comp(Packer.Data(), pExpected, Size), 0); } From f6015be263711b4f565805481cf9b9f2e2b69370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 3 May 2023 18:17:27 +0200 Subject: [PATCH 3/5] Add default parameter for `CPacker::AddString` --- src/engine/shared/packer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/shared/packer.h b/src/engine/shared/packer.h index 2c428a1e3..7b633c417 100644 --- a/src/engine/shared/packer.h +++ b/src/engine/shared/packer.h @@ -20,7 +20,7 @@ private: public: void Reset(); void AddInt(int i); - void AddString(const char *pStr, int Limit); + void AddString(const char *pStr, int Limit = PACKER_BUFFER_SIZE); void AddRaw(const void *pData, int Size); int Size() const { return (int)(m_pCurrent - m_aBuffer); } From 8794cc9f9ce94b49d302308ea337b80d71d74a7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 3 May 2023 18:17:58 +0200 Subject: [PATCH 4/5] Add tests for packer error handling --- src/test/packer.cpp | 54 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/test/packer.cpp b/src/test/packer.cpp index 90126c365..6b4e13369 100644 --- a/src/test/packer.cpp +++ b/src/test/packer.cpp @@ -242,3 +242,57 @@ TEST(Packer, AddStringBroken) ExpectAddString5("\x80\x80", 5, "�"); ExpectAddString5("\x80\x80", 6, 0); } + +TEST(Packer, Error) +{ + char aData[CPacker::PACKER_BUFFER_SIZE]; + mem_zero(aData, sizeof(aData)); + + { + CPacker Packer; + Packer.Reset(); + EXPECT_EQ(Packer.Error(), false); + Packer.AddRaw(aData, sizeof(aData) - 1); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData) - 1); + Packer.AddInt(1); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData)); + Packer.AddInt(2); + EXPECT_EQ(Packer.Error(), true); + Packer.AddInt(3); + EXPECT_EQ(Packer.Error(), true); + } + + { + CPacker Packer; + Packer.Reset(); + EXPECT_EQ(Packer.Error(), false); + Packer.AddRaw(aData, sizeof(aData) - 1); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData) - 1); + Packer.AddRaw(aData, 1); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData)); + Packer.AddRaw(aData, 1); + EXPECT_EQ(Packer.Error(), true); + Packer.AddRaw(aData, 1); + EXPECT_EQ(Packer.Error(), true); + } + + { + CPacker Packer; + Packer.Reset(); + EXPECT_EQ(Packer.Error(), false); + Packer.AddRaw(aData, sizeof(aData) - 5); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData) - 5); + Packer.AddString("test"); + EXPECT_EQ(Packer.Error(), false); + EXPECT_EQ(Packer.Size(), sizeof(aData)); + Packer.AddString("test"); + EXPECT_EQ(Packer.Error(), true); + Packer.AddString("test"); + EXPECT_EQ(Packer.Error(), true); + } +} From c5e336f5e24cc63e800da686b400700e90dc0d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 3 May 2023 18:19:04 +0200 Subject: [PATCH 5/5] Fix off-by-one error when adding raw data to buffer One byte too few could be added to packers as raw data. Detected by the new test cases. --- src/engine/shared/packer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/shared/packer.cpp b/src/engine/shared/packer.cpp index a8a4de6a6..2e64bbc3c 100644 --- a/src/engine/shared/packer.cpp +++ b/src/engine/shared/packer.cpp @@ -66,7 +66,7 @@ void CPacker::AddRaw(const void *pData, int Size) if(m_Error) return; - if(m_pCurrent + Size >= m_pEnd) + if(m_pCurrent + Size > m_pEnd) { m_Error = true; return;