Skip to content

Commit bb52797

Browse files
SergeyMakeevSergeyMakeev
committed
Refactor: prevent potential memory stomps in BlobBuilder and fix array safety
Fixes unsafe memory access in BlobBuilder that could lead to memory corruption. Updates the Array API to eliminate the possibility of dangling pointers and references.
1 parent 2502cfa commit bb52797

File tree

11 files changed

+318
-118
lines changed

11 files changed

+318
-118
lines changed

‎Zmeya.h‎

Lines changed: 149 additions & 83 deletions
Large diffs are not rendered by default.

‎ZmeyaTest01.cpp‎

Lines changed: 122 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,61 @@
1+
#include "gtest/gtest.h"
2+
3+
namespace Memory
4+
{
5+
6+
size_t mallocCount = 0;
7+
size_t freeCount = 0;
8+
9+
struct Header
10+
{
11+
void* p;
12+
size_t size;
13+
size_t magic;
14+
};
15+
16+
const size_t kMinValidAlignment = 4;
17+
18+
void* malloc(size_t bytesCount, size_t alignment)
19+
{
20+
mallocCount++;
21+
if (alignment < kMinValidAlignment)
22+
{
23+
alignment = kMinValidAlignment;
24+
}
25+
void* p;
26+
void** p2;
27+
size_t offset = alignment - 1 + sizeof(Header);
28+
if ((p = (void*)std::malloc(bytesCount + offset)) == NULL)
29+
{
30+
return NULL;
31+
}
32+
p2 = (void**)(((size_t)(p) + offset) & ~(alignment - 1));
33+
34+
Header* h = reinterpret_cast<Header*>(reinterpret_cast<char*>(p2) - sizeof(Header));
35+
h->p = p;
36+
h->size = bytesCount;
37+
h->magic = 0x13061979;
38+
return p2;
39+
}
40+
41+
void mfree(void* p)
42+
{
43+
freeCount++;
44+
if (!p)
45+
{
46+
return;
47+
}
48+
Header* h = reinterpret_cast<Header*>(reinterpret_cast<char*>(p) - sizeof(Header));
49+
ASSERT_EQ(h->magic, 0x13061979);
50+
std::free(h->p);
51+
}
52+
53+
} // namespace Memory
54+
55+
#define ZMEYA_ALLOC(sizeInBytes, alignment) Memory::malloc(sizeInBytes, alignment)
56+
#define ZMEYA_FREE(ptr) Memory::mfree(ptr)
157
#include "TestHelper.h"
258
#include "Zmeya.h"
3-
#include "gtest/gtest.h"
459

560
struct SimpleTestRoot
661
{
@@ -11,8 +66,6 @@ struct SimpleTestRoot
1166
uint32_t arr[32];
1267
};
1368

14-
15-
1669
static void validate(const SimpleTestRoot* root)
1770
{
1871
EXPECT_FLOAT_EQ(root->a, 13.0f);
@@ -30,7 +83,7 @@ TEST(ZmeyaTestSuite, SimpleTest)
3083
std::vector<char> bytesCopy;
3184
{
3285
// create blob
33-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
86+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
3487

3588
// allocate structure
3689
zm::BlobPtr<SimpleTestRoot> root = blobBuilder->allocate<SimpleTestRoot>();
@@ -61,3 +114,68 @@ TEST(ZmeyaTestSuite, SimpleTest)
61114
const SimpleTestRoot* rootCopy = (const SimpleTestRoot*)(bytesCopy.data());
62115
validate(rootCopy);
63116
}
117+
118+
struct Desc
119+
{
120+
zm::String name;
121+
float v1;
122+
uint32_t v2;
123+
};
124+
125+
struct TestRoot
126+
{
127+
zm::Array<Desc> arr;
128+
};
129+
130+
TEST(ZmeyaTestSuite, SimpleTest2)
131+
{
132+
const std::vector<std::string> names = {"apple", "banana", "orange", "castle", "dragon", "flower", "guitar",
133+
"hockey", "island", "jungle", "kingdom", "library", "monster", "notable",
134+
"oceanic", "painter", "quarter", "rescue", "seventh", "trivial", "umbrella",
135+
"village", "warrior", "xenial", "yonder", "zephyr"};
136+
137+
Memory::mallocCount = 0;
138+
Memory::freeCount = 0;
139+
EXPECT_EQ(Memory::mallocCount, 0);
140+
EXPECT_EQ(Memory::freeCount, 0);
141+
142+
std::vector<char> blob;
143+
{
144+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
145+
zm::BlobPtr<TestRoot> root = blobBuilder->allocate<TestRoot>();
146+
147+
blobBuilder->resizeArray(root->arr, names.size());
148+
EXPECT_EQ(root->arr.size(), names.size());
149+
150+
for (size_t i = 0; i < names.size(); i++)
151+
{
152+
zm::BlobPtr<Desc> desc = blobBuilder->getArrayElement(root->arr, i);
153+
blobBuilder->copyTo(desc->name, names[i].c_str());
154+
const char* s1 = root->arr[i].name.c_str();
155+
const char* s2 = names[i].c_str();
156+
EXPECT_STREQ(s1, s2);
157+
desc->v1 = (float)(i);
158+
desc->v2 = (uint32_t)(i);
159+
}
160+
zm::Span<char> bytes = blobBuilder->finalize();
161+
blob = std::vector<char>(bytes.data, bytes.data + bytes.size);
162+
}
163+
164+
EXPECT_GT(Memory::mallocCount, 0);
165+
EXPECT_GT(Memory::freeCount, 0);
166+
EXPECT_EQ(Memory::mallocCount, Memory::freeCount);
167+
168+
// validate
169+
const TestRoot* rootCopy = (const TestRoot*)(blob.data());
170+
EXPECT_EQ(rootCopy->arr.size(), names.size());
171+
172+
for (size_t i = 0; i < names.size(); i++)
173+
{
174+
const Desc& desc = rootCopy->arr[i];
175+
EXPECT_STREQ(desc.name.c_str(), names[i].c_str());
176+
EXPECT_FLOAT_EQ(desc.v1, (float)(i));
177+
EXPECT_EQ(desc.v2, (uint32_t)(i));
178+
}
179+
180+
EXPECT_EQ(Memory::mallocCount, Memory::freeCount);
181+
}

‎ZmeyaTest02.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ TEST(ZmeyaTestSuite, PointerTest)
3535
{
3636
std::vector<char> bytesCopy;
3737
{
38-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
38+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
3939

4040
zm::BlobPtr<PointerTestRoot> root = blobBuilder->allocate<PointerTestRoot>();
4141
zm::BlobPtr<PointerTestNode> nodeLeft = blobBuilder->allocate<PointerTestNode>();

‎ZmeyaTest03.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ TEST(ZmeyaTestSuite, ArrayTest)
9999
{
100100
std::vector<char> bytesCopy;
101101
{
102-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
102+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
103103
zm::BlobPtr<ArrayTestRoot> root = blobBuilder->allocate<ArrayTestRoot>();
104104

105105
// assign from std::vector
@@ -116,17 +116,18 @@ TEST(ZmeyaTestSuite, ArrayTest)
116116
// resize array
117117
blobBuilder->resizeArray(root->arr4, 4);
118118
// assign array elements (sub-arrays)
119-
blobBuilder->copyTo(root->arr4[0], {1.2f, 2.3f});
120-
blobBuilder->copyTo(root->arr4[1], {7.1f, 8.8f, 3.2f});
121-
blobBuilder->copyTo(root->arr4[2], {16.0f, 12.0f, 99.5f, -143.0f});
122-
blobBuilder->copyTo(root->arr4[3], {-1.0f});
119+
120+
blobBuilder->copyTo(blobBuilder->getArrayElement(root->arr4, 0), {1.2f, 2.3f});
121+
blobBuilder->copyTo(blobBuilder->getArrayElement(root->arr4, 1), {7.1f, 8.8f, 3.2f});
122+
blobBuilder->copyTo(blobBuilder->getArrayElement(root->arr4, 2), {16.0f, 12.0f, 99.5f, -143.0f});
123+
blobBuilder->copyTo(blobBuilder->getArrayElement(root->arr4, 3), {-1.0f});
123124

124125
// resize array
125126
blobBuilder->resizeArray(root->arr5, 793);
126127
for (size_t i = 0; i < root->arr5.size(); i++)
127128
{
128129
zm::BlobPtr<Payload> payload = blobBuilder->allocate<Payload>(1.3f + float(i) * 0.4f, uint32_t(i) + 3);
129-
root->arr5[i] = payload;
130+
*blobBuilder->getArrayElement(root->arr5, i) = payload;
130131
}
131132

132133
std::vector<std::vector<uint32_t>> vec2 = {{1, 2}, {2, 7, 11, 9, 141}, {15, 9, 33, 7}};

‎ZmeyaTest05.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ TEST(ZmeyaTestSuite, StringTest)
5858
{
5959
std::vector<char> bytesCopy;
6060
{
61-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
61+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
6262
zm::BlobPtr<StringTestRoot> root = blobBuilder->allocate<StringTestRoot>();
6363

6464
// assign from null-terminated c string (74 bytes long)
@@ -89,7 +89,7 @@ TEST(ZmeyaTestSuite, StringTest)
8989
blobBuilder->resizeArray(root->strArr4, numStrings);
9090
for (size_t i = 0; i < root->strArr4.size(); i++)
9191
{
92-
blobBuilder->referTo(root->strArr4[i], root->str1);
92+
blobBuilder->referTo(blobBuilder->getArrayElement(root->strArr4, i), root->str1);
9393
}
9494

9595
validate(root.get());

‎ZmeyaTest06.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ TEST(ZmeyaTestSuite, HashSetTest)
7070
{
7171
std::vector<char> bytesCopy;
7272
{
73-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
73+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
7474
zm::BlobPtr<HashSetTestRoot> root = blobBuilder->allocate<HashSetTestRoot>();
7575

7676
// assign from std::unordered_set

‎ZmeyaTest07.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ TEST(ZmeyaTestSuite, HashMapTest)
8787
{
8888
std::vector<char> bytesCopy;
8989
{
90-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
90+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
9191
zm::BlobPtr<HashMapTestRoot> root = blobBuilder->allocate<HashMapTestRoot>();
9292

9393
// assign from std::unordered_map

‎ZmeyaTest08.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ TEST(ZmeyaTestSuite, IteratorsTest)
5555
{
5656
std::vector<char> bytesCopy;
5757
{
58-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
58+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
5959
zm::BlobPtr<IteratorsTestRoot> root = blobBuilder->allocate<IteratorsTestRoot>();
6060

6161
blobBuilder->copyTo(root->arr, {10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0});

‎ZmeyaTest09.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,20 +79,20 @@ static void generateTestFile(const char* fileName)
7979
{
8080
std::vector<std::string> objectNames = {"root", "test1", "floor", "window", "arrow", "door"};
8181

82-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
82+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
8383
zm::BlobPtr<SimpleFileTestRoot> root = blobBuilder->allocate<SimpleFileTestRoot>();
8484
root->magic = 0x59454D5A;
8585
blobBuilder->resizeArray(root->objects, 6);
8686
for (size_t i = 0; i < root->objects.size(); i++)
8787
{
88-
Object& object = root->objects[i];
89-
blobBuilder->copyTo(object.name, objectNames[i]);
90-
object.position = Vec2(float(i), float(i + 4));
88+
zm::BlobPtr<Object> object = blobBuilder->getArrayElement(root->objects, i);
89+
blobBuilder->copyTo(object->name, objectNames[i]);
90+
object->position = Vec2(float(i), float(i + 4));
9191

9292
if (i > 0)
9393
{
9494
const Object& parentObject = root->objects[i - 1];
95-
blobBuilder->assignTo(object.parent, parentObject);
95+
blobBuilder->assignTo(object->parent, parentObject);
9696
}
9797
}
9898

‎ZmeyaTest10.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ void createChildren(zm::BlobBuilder* blobBuilder, const zm::BlobPtr<MMapTestNode
141141
blobBuilder->copyTo(node->name, "leaf_" + std::to_string(startIndex + i));
142142
node->payload = uint32_t(count + startIndex * 13);
143143
node->parent = parent;
144-
parent->children[i] = node;
144+
zm::BlobPtr<zm::Pointer<MMapTestNode>> ch = blobBuilder->getArrayElement(parent->children, i);
145+
*ch = node;
145146
}
146147
}
147148

@@ -173,7 +174,7 @@ zm::BlobPtr<MMapTestNode> allocateNode2(zm::BlobBuilder* blobBuilder, const zm::
173174

174175
static void generateTestFile(const char* fileName)
175176
{
176-
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create();
177+
std::shared_ptr<zm::BlobBuilder> blobBuilder = zm::BlobBuilder::create(1);
177178
zm::BlobPtr<MMapTestRoot> root = blobBuilder->allocate<MMapTestRoot>();
178179
root->magic = 0x59454D5A;
179180
blobBuilder->copyTo(root->desc, "Zmyea test file. This is supposed to be a long enough string. I think it is long enough now.");
@@ -191,7 +192,8 @@ static void generateTestFile(const char* fileName)
191192
{
192193
rootNode = allocateNode2(blobBuilder.get(), root, i);
193194
}
194-
root->roots[i] = rootNode;
195+
zm::BlobPtr<zm::Pointer<MMapTestNode>> rt = blobBuilder->getArrayElement(root->roots, i);
196+
*rt = rootNode;
195197
}
196198

197199
validate(root.get());

0 commit comments

Comments
 (0)