Merge #6557

6557: Add tests for `CPacker` error handling, fix minor bug, minor refactoring r=def- a=Robyt3

Closes #6525.

## Checklist

- [X] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [X] Written a unit test (especially base/) or added coverage to integration test
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] 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>

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);
@@ -66,9 +66,9 @@ 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 = 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;
@@ -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;
+		m_Error = true;
 		return 0;
 	}
 
 	int i = *(int *)m_pCurrent;
-	m_pCurrent += 4;
+	m_pCurrent += sizeof(int);
 	return i;
 }
 
@@ -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;
 	}
 

src/engine/shared/packer.h

@@ -15,12 +15,12 @@ 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();
 	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); }
@@ -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

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);
 }
@@ -242,3 +242,57 @@ TEST(Packer, AddStringBroken)
 	ExpectAddString5("\x80\x80", 5, "<EFBFBD>");
 	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);
+	}
+}