Skip to content

Commit

Permalink
Make DynArray support objects larger than 1 gigabyte
Browse files Browse the repository at this point in the history
The expression ``const int newAllocated = cap * 2;`` easily causes
overflow, as soon as the input is 1.0 GiB. This goes unnoticed because
release builds of tinyxml2 do not have active assertions.

The change in commit 9.0.0-20-g8fd6cc6 did not do anything useful;
the signed multiplication overflow (and thus undefined behavior)
still occurs.

Using ``int`` in this class is really archaic, because it limits the
class to a gigabyte even on 64-bit platforms.

The multiplication overflow check also needs to include sizeof(T),
otherwise you can run into unsigned multiplication overflow (defined,
but undesirable) in the memcpy() call.

testcase:

int main()
{
        tinyxml2::XMLDocument doc;
        doc.InsertEndChild(doc.NewDeclaration());
        auto root = doc.NewElement("root");
        size_t sz = 0x80000001;
        auto blank = new char[sz];
        memset(blank, ' ', sz);
        blank[sz-1]='\0';
        root->SetText(blank);
        doc.InsertEndChild(root);
        tinyxml2::XMLPrinter printer(nullptr);
        doc.Print(&printer);
}
  • Loading branch information
jengelh committed Jul 1, 2024
1 parent 3a893e5 commit eb3ab0d
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 34 deletions.
2 changes: 1 addition & 1 deletion tinyxml2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2221,7 +2221,7 @@ void XMLDocument::MarkInUse(const XMLNode* const node)
TIXMLASSERT(node);
TIXMLASSERT(node->_parent == 0);

for (int i = 0; i < _unlinked.Size(); ++i) {
for (size_t i = 0; i < _unlinked.Size(); ++i) {
if (node == _unlinked[i]) {
_unlinked.SwapRemove(i);
break;
Expand Down
65 changes: 32 additions & 33 deletions tinyxml2.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class TINYXML2_LIB StrPair
Has a small initial memory pool, so that low or no usage will not
cause a call to new/delete
*/
template <class T, int INITIAL_SIZE>
template <class T, size_t INITIAL_SIZE>
class DynArray
{
public:
Expand Down Expand Up @@ -227,9 +227,8 @@ class DynArray
++_size;
}

T* PushArr( int count ) {
TIXMLASSERT( count >= 0 );
TIXMLASSERT( _size <= INT_MAX - count );
T* PushArr( size_t count ) {
TIXMLASSERT( _size <= SIZE_MAX - count );
EnsureCapacity( _size+count );
T* ret = &_mem[_size];
_size += count;
Expand All @@ -242,7 +241,7 @@ class DynArray
return _mem[_size];
}

void PopArr( int count ) {
void PopArr( size_t count ) {
TIXMLASSERT( _size >= count );
_size -= count;
}
Expand All @@ -251,13 +250,13 @@ class DynArray
return _size == 0;
}

T& operator[](int i) {
TIXMLASSERT( i>= 0 && i < _size );
T& operator[](size_t i) {
TIXMLASSERT( i < _size );
return _mem[i];
}

const T& operator[](int i) const {
TIXMLASSERT( i>= 0 && i < _size );
const T& operator[](size_t i) const {
TIXMLASSERT( i < _size );
return _mem[i];
}

Expand All @@ -266,18 +265,18 @@ class DynArray
return _mem[ _size - 1];
}

int Size() const {
size_t Size() const {
TIXMLASSERT( _size >= 0 );
return _size;
}

int Capacity() const {
size_t Capacity() const {
TIXMLASSERT( _allocated >= INITIAL_SIZE );
return _allocated;
}

void SwapRemove(int i) {
TIXMLASSERT(i >= 0 && i < _size);
void SwapRemove(size_t i) {
TIXMLASSERT(i < _size);
TIXMLASSERT(_size > 0);
_mem[i] = _mem[_size - 1];
--_size;
Expand All @@ -297,14 +296,14 @@ class DynArray
DynArray( const DynArray& ); // not supported
void operator=( const DynArray& ); // not supported

void EnsureCapacity( int cap ) {
void EnsureCapacity( size_t cap ) {
TIXMLASSERT( cap > 0 );
if ( cap > _allocated ) {
TIXMLASSERT( cap <= INT_MAX / 2 );
const int newAllocated = cap * 2;
T* newMem = new T[static_cast<unsigned int>(newAllocated)];
TIXMLASSERT( cap <= SIZE_MAX / 2 / sizeof(T));
const size_t newAllocated = cap * 2;
T* newMem = new T[newAllocated];
TIXMLASSERT( newAllocated >= _size );
memcpy( newMem, _mem, sizeof(T)*static_cast<size_t>(_size) ); // warning: not using constructors, only works for PODs
memcpy( newMem, _mem, sizeof(T) * _size ); // warning: not using constructors, only works for PODs
if ( _mem != _pool ) {
delete [] _mem;
}
Expand All @@ -314,9 +313,9 @@ class DynArray
}

T* _mem;
T _pool[static_cast<size_t>(INITIAL_SIZE)];
int _allocated; // objects allocated
int _size; // number objects in use
T _pool[INITIAL_SIZE];
size_t _allocated; // objects allocated
size_t _size; // number objects in use
};


Expand All @@ -330,7 +329,7 @@ class MemPool
MemPool() {}
virtual ~MemPool() {}

virtual int ItemSize() const = 0;
virtual size_t ItemSize() const = 0;
virtual void* Alloc() = 0;
virtual void Free( void* ) = 0;
virtual void SetTracked() = 0;
Expand All @@ -340,7 +339,7 @@ class MemPool
/*
Template child class to create pools of the correct type.
*/
template< int ITEM_SIZE >
template< size_t ITEM_SIZE >
class MemPoolT : public MemPool
{
public:
Expand All @@ -362,10 +361,10 @@ class MemPoolT : public MemPool
_nUntracked = 0;
}

virtual int ItemSize() const override{
virtual size_t ItemSize() const override {
return ITEM_SIZE;
}
int CurrentAllocs() const {
size_t CurrentAllocs() const {
return _currentAllocs;
}

Expand All @@ -376,7 +375,7 @@ class MemPoolT : public MemPool
_blockPtrs.Push( block );

Item* blockItems = block->items;
for( int i = 0; i < ITEMS_PER_BLOCK - 1; ++i ) {
for( size_t i = 0; i < ITEMS_PER_BLOCK - 1; ++i ) {
blockItems[i].next = &(blockItems[i + 1]);
}
blockItems[ITEMS_PER_BLOCK - 1].next = 0;
Expand Down Expand Up @@ -417,7 +416,7 @@ class MemPoolT : public MemPool
--_nUntracked;
}

int Untracked() const {
size_t Untracked() const {
return _nUntracked;
}

Expand Down Expand Up @@ -448,10 +447,10 @@ class MemPoolT : public MemPool
DynArray< Block*, 10 > _blockPtrs;
Item* _root;

int _currentAllocs;
int _nAllocs;
int _maxAllocs;
int _nUntracked;
size_t _currentAllocs;
size_t _nAllocs;
size_t _maxAllocs;
size_t _nUntracked;
};


Expand Down Expand Up @@ -1981,11 +1980,11 @@ class TINYXML2_LIB XMLDocument : public XMLNode
void PushDepth();
void PopDepth();

template<class NodeType, int PoolElementSize>
template<class NodeType, size_t PoolElementSize>
NodeType* CreateUnlinkedNode( MemPoolT<PoolElementSize>& pool );
};

template<class NodeType, int PoolElementSize>
template<class NodeType, size_t PoolElementSize>
inline NodeType* XMLDocument::CreateUnlinkedNode( MemPoolT<PoolElementSize>& pool )
{
TIXMLASSERT( sizeof( NodeType ) == PoolElementSize );
Expand Down

0 comments on commit eb3ab0d

Please sign in to comment.