Skip to content

Commit 461bc3b

Browse files
authored
Stop managed_array from accidentally changing C++ types (#155)
Storing non-POD types in char buffers is unsafe. * Ensure alignments are compatible with desired type. * Ensure raw data is copied with `memcpy` Otherwise, `_list_handouts` placement-constructs entries as one type (`list_impl`) which is changed when they're copied by the copy constructor of T (`list_interface`, different type). For POD data this wouldn't matter, but here it changes the vtable pointer and indirectly causes an OOB write. This crashed my memory allocator, which is how I spotted it.
1 parent 645f5a6 commit 461bc3b

2 files changed

Lines changed: 17 additions & 5 deletions

File tree

inkcpp/array.h

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ class managed_array : public snapshot_interface
3535
if constexpr (dynamic) {
3636
if constexpr (simple) {
3737
_dynamic_data = reinterpret_cast<T*>(new char[sizeof(T) * initialCapacity]);
38+
inkAssert(( ::size_t ) _dynamic_data % alignof(T) == 0);
3839
} else {
3940
_dynamic_data = new T[initialCapacity];
4041
}
@@ -113,6 +114,7 @@ class managed_array : public snapshot_interface
113114
inkAssert(_size <= _capacity, "Try to append to a full array!");
114115
// TODO(JBenda): Silent fail?
115116
}
117+
inkAssert(_size < _capacity);
116118
return data()[_size++];
117119
}
118120

@@ -197,8 +199,8 @@ class managed_array : public snapshot_interface
197199

198200
private:
199201
T* _dynamic_data = nullptr;
200-
size_t _capacity;
201-
size_t _size;
202+
size_t _capacity = 0;
203+
size_t _size = 0;
202204
if_t<dynamic, char, T> _static_data[dynamic ? 1 : initialCapacity];
203205
};
204206

@@ -273,13 +275,22 @@ void managed_array<T, dynamic, initialCapacity, simple>::extend(size_t capacity)
273275
}
274276
T* new_data = nullptr;
275277
if constexpr (simple) {
278+
// Warning: Allocating typed data in a char* container is potentially unsafe. We need to be sure
279+
// the alignment is compatible with the destination type...
276280
new_data = reinterpret_cast<T*>(new char[sizeof(T) * new_capacity]);
281+
inkAssert(( ::size_t ) new_data % alignof(T) == 0);
282+
283+
// ...and we have to copy the contents byte-by-byte, since client code (_list_handouts)
284+
// type-puns between two classes with different vtbls here. Copying these elementwise would
285+
// change the stored C++ type.
286+
memcpy(new_data, _dynamic_data, sizeof(T) * _capacity);
277287
} else {
288+
// Allocate and copy typed data normally
278289
new_data = new T[new_capacity];
279-
}
280290

281-
for (size_t i = 0; i < _capacity; ++i) {
282-
new_data[i] = _dynamic_data[i];
291+
for (size_t i = 0; i < _capacity; ++i) {
292+
new_data[i] = _dynamic_data[i];
293+
}
283294
}
284295

285296
if constexpr (simple) {

inkcpp/list_table.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,6 +825,7 @@ list_table::list list_table::redefine(list lh, list rh)
825825
list_interface* list_table::handout_list(list l)
826826
{
827827
static_assert(sizeof(list_interface) == sizeof(list_impl));
828+
static_assert(alignof(list_interface) == alignof(list_impl));
828829
auto& res = _list_handouts.push();
829830
new (&res) list_impl(*this, l.lid);
830831
return &res;

0 commit comments

Comments
 (0)