edg-l/ddnet
1
0
Fork 0
mirror of https://github.com/ddnet/ddnet.git synced 2024-11-10 10:08:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge #4536

Browse source 
4536: Fix invalid left shift in CVariableInt::Unpack, add tests for CVariableInt r=def- a=Robyt3

The last iteration in `CVariableInt::Unpack` is changed from

```cpp
*pInOut |= (*pSrc & (0x1F)) << (6 + 7 + 7 + 7);
```
to
```cpp
*pInOut |= (*pSrc & 0x0F) << (6 + 7 + 7 + 7);
```

which means the last iteration will unpack at most 4 more bits instead of 5, hence invalid left shifts by 27 places that could otherwise occur on this line are prevented (closes #3686). Now at most 31 bits are unpacked, in addition to the sign bit.

Tests are added to ensure correct functionality of all `CVariableInt` functions.

## 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 if it works standalone, system.c especially
- [X] Considered possible null pointers and out of bounds array indexing
- [X] 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 <robert.mueller@uni-siegen.de>
This commit is contained in:
bors[bot] 2021-12-27 22:08:10 +00:00 committed by GitHub
parent a9cd29d624 2ffd0ddae2
commit 584695424f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 104 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CMakeLists.txt
View file

@ -2276,6 +2276,7 @@ if(GTEST_FOUND OR DOWNLOAD_GTEST)
bezier.cpp
blocklist_driver.cpp
color.cpp
compression.cpp
csv.cpp
datafile.cpp
fs.cpp

46
src/engine/shared/compression.cpp
View file

@ -8,17 +8,17 @@
unsigned char *CVariableInt::Pack(unsigned char *pDst, int i)
{
*pDst = (i >> 25) & 0x40; // set sign bit if i<0
i = i ^ (i >> 31); // if(i<0) i = ~i
i ^= i >> 31; // if(i<0) i = ~i
*pDst |= i & 0x3F; // pack 6bit into dst
i >>= 6; // discard 6 bits
if(i)
{
*pDst |= 0x80; // set extend bit
while(1)
while(true)
{
pDst++;
*pDst = i & (0x7F); // pack 7bit
*pDst = i & 0x7F; // pack 7bit
i >>= 7; // discard 7 bits
*pDst |= (i != 0) << 7; // set extend bit (may branch)
if(!i)
@ -32,7 +32,7 @@ unsigned char *CVariableInt::Pack(unsigned char *pDst, int i)
const unsigned char *CVariableInt::Unpack(const unsigned char *pSrc, int *pInOut)
{
int Sign = (*pSrc >> 6) & 1;
const int Sign = (*pSrc >> 6) & 1;
*pInOut = *pSrc & 0x3F;
do
@ -40,35 +40,37 @@ const unsigned char *CVariableInt::Unpack(const unsigned char *pSrc, int *pInOut
if(!(*pSrc & 0x80))
break;
pSrc++;
*pInOut |= (*pSrc & (0x7F)) << (6);
*pInOut |= (*pSrc & 0x7F) << 6;
if(!(*pSrc & 0x80))
break;
pSrc++;
*pInOut |= (*pSrc & (0x7F)) << (6 + 7);
*pInOut |= (*pSrc & 0x7F) << (6 + 7);
if(!(*pSrc & 0x80))
break;
pSrc++;
*pInOut |= (*pSrc & (0x7F)) << (6 + 7 + 7);
*pInOut |= (*pSrc & 0x7F) << (6 + 7 + 7);
if(!(*pSrc & 0x80))
break;
pSrc++;
*pInOut |= (*pSrc & (0x1F)) << (6 + 7 + 7 + 7);
} while(0);
*pInOut |= (*pSrc & 0x0F) << (6 + 7 + 7 + 7);
} while(false);
pSrc++;
*pInOut ^= -Sign; // if(sign) *i = ~(*i)
return pSrc;
}
long CVariableInt::Decompress(const void *pSrc_, int Size, void *pDst_, int DstSize)
long CVariableInt::Decompress(const void *pSrc_, int SrcSize, void *pDst_, int DstSize)
{
dbg_assert(DstSize % sizeof(int) == 0, "invalid bounds");
const unsigned char *pSrc = (unsigned char *)pSrc_;
const unsigned char *pEnd = pSrc + Size;
const unsigned char *pEnd = pSrc + SrcSize;
int *pDst = (int *)pDst_;
int *pDstEnd = pDst + DstSize / 4;
const int *pDstEnd = pDst + DstSize / sizeof(int);
while(pSrc < pEnd)
{
if(pDst >= pDstEnd)
@ -76,22 +78,24 @@ long CVariableInt::Decompress(const void *pSrc_, int Size, void *pDst_, int DstS
pSrc = CVariableInt::Unpack(pSrc, pDst);
pDst++;
}
return (unsigned char *)pDst - (unsigned char *)pDst_;
return (long)((unsigned char *)pDst - (unsigned char *)pDst_);
}
long CVariableInt::Compress(const void *pSrc_, int Size, void *pDst_, int DstSize)
long CVariableInt::Compress(const void *pSrc_, int SrcSize, void *pDst_, int DstSize)
{
int *pSrc = (int *)pSrc_;
dbg_assert(SrcSize % sizeof(int) == 0, "invalid bounds");
const int *pSrc = (int *)pSrc_;
unsigned char *pDst = (unsigned char *)pDst_;
unsigned char *pDstEnd = pDst + DstSize;
Size /= 4;
while(Size)
const unsigned char *pDstEnd = pDst + DstSize;
SrcSize /= sizeof(int);
while(SrcSize)
{
if(pDstEnd - pDst < 6)
if(pDstEnd - pDst <= MAX_BYTES_PACKED)
return -1;
pDst = CVariableInt::Pack(pDst, *pSrc);
Size--;
SrcSize--;
pSrc++;
}
return pDst - (unsigned char *)pDst_;
return (long)(pDst - (unsigned char *)pDst_);
}

12
src/engine/shared/compression.h
View file

@ -2,13 +2,21 @@
/* If you are missing that file, acquire a complete release at teeworlds.com. */
#ifndef ENGINE_SHARED_COMPRESSION_H
#define ENGINE_SHARED_COMPRESSION_H
// variable int packing
class CVariableInt
{
public:
enum
{
MAX_BYTES_PACKED = 5, // maximum number of bytes in a packed int
};
static unsigned char *Pack(unsigned char *pDst, int i);
static const unsigned char *Unpack(const unsigned char *pSrc, int *pInOut);
static long Compress(const void *pSrc, int Size, void *pDst, int DstSize);
static long Decompress(const void *pSrc, int Size, void *pDst, int DstSize);
static long Compress(const void *pSrc, int SrcSize, void *pDst, int DstSize);
static long Decompress(const void *pSrc, int SrcSize, void *pDst, int DstSize);
};
#endif

68
src/test/compression.cpp Normal file
View file

@ -0,0 +1,68 @@
#include <gtest/gtest.h>
#include <engine/shared/compression.h>
static const int DATA[] = {0, 1, -1, 32, 64, 256, -512, 12345, -123456, 1234567, 12345678, 123456789, 2147483647, (-2147483647 - 1)};
static const int NUM = sizeof(DATA) / sizeof(int);
static const int SIZES[NUM] = {1, 1, 1, 1, 2, 2, 2, 3, 3, 4, 4, 4, 5, 5};
TEST(CVariableInt, RoundtripPackUnpack)
{
for(int i = 0; i < NUM; i++)
{
unsigned char aPacked[CVariableInt::MAX_BYTES_PACKED];
int Result;
EXPECT_EQ(int(CVariableInt::Pack(aPacked, DATA[i]) - aPacked), SIZES[i]);
EXPECT_EQ(int(CVariableInt::Unpack(aPacked, &Result) - aPacked), SIZES[i]);
EXPECT_EQ(Result, DATA[i]);
}
}
TEST(CVariableInt, UnpackInvalid)
{
unsigned char aPacked[CVariableInt::MAX_BYTES_PACKED];
for(int i = 0; i < CVariableInt::MAX_BYTES_PACKED; i++)
aPacked[i] = 0xFF;
int Result;
EXPECT_EQ(int(CVariableInt::Unpack(aPacked, &Result) - aPacked), int(CVariableInt::MAX_BYTES_PACKED));
EXPECT_EQ(Result, (-2147483647 - 1));
aPacked[0] &= ~0x40; // unset sign bit
EXPECT_EQ(int(CVariableInt::Unpack(aPacked, &Result) - aPacked), int(CVariableInt::MAX_BYTES_PACKED));
EXPECT_EQ(Result, 2147483647);
}
TEST(CVariableInt, RoundtripCompressDecompress)
{
unsigned char aCompressed[NUM * CVariableInt::MAX_BYTES_PACKED];
int aDecompressed[NUM];
long ExpectedCompressedSize = 0;
for(int i = 0; i < NUM; i++)
ExpectedCompressedSize += SIZES[i];
long CompressedSize = CVariableInt::Compress(DATA, sizeof(DATA), aCompressed, sizeof(aCompressed));
ASSERT_EQ(CompressedSize, ExpectedCompressedSize);
long DecompressedSize = CVariableInt::Decompress(aCompressed, ExpectedCompressedSize, aDecompressed, sizeof(aDecompressed));
ASSERT_EQ(DecompressedSize, sizeof(DATA));
for(int i = 0; i < NUM; i++)
{
EXPECT_EQ(DATA[i], aDecompressed[i]);
}
}
TEST(CVariableInt, CompressBufferTooSmall)
{
unsigned char aCompressed[NUM]; // too small
long CompressedSize = CVariableInt::Compress(DATA, sizeof(DATA), aCompressed, sizeof(aCompressed));
ASSERT_EQ(CompressedSize, -1);
}
TEST(CVariableInt, DecompressBufferTooSmall)
{
unsigned char aCompressed[] = {0x00, 0x01, 0x40, 0x20, 0x80, 0x01, 0x80, 0x04, 0xFF, 0x07, 0xB9, 0xC0, 0x01};
int aUncompressed[4]; // too small
long CompressedSize = CVariableInt::Decompress(aCompressed, sizeof(aCompressed), aUncompressed, sizeof(aUncompressed));
ASSERT_EQ(CompressedSize, -1);
}