Skip to content

bugfix(network): Prevent out of bounds memory access in NetCommandWrapperListNode::copyChunkData() #1946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xezon merged 1 commit into from
Dec 7, 2025
+34 −14
Merged

bugfix(network): Prevent out of bounds memory access in NetCommandWrapperListNode::copyChunkData() #1946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Core/GameEngine/Include/GameNetwork/NetCommandWrapperList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class NetCommandWrapperListNode : public MemoryPoolObject
protected:
UnsignedShort m_commandID;
UnsignedByte *m_data;
UnsignedInt m_dataLength;
UnsignedInt m_totalDataLength;
Copy link
Author

@Mauller Mauller Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I altered this for sanity reasons as the original name can get confusing when the data length of the data chunk is also being considered.

I did consider further refactoring the NetWrapperCommandMsg but chose to revert those changes for now as they were touching NetPacket which has some waiting PR's to be merged

Bool *m_chunksPresent;
UnsignedInt m_numChunks;
UnsignedInt m_numChunksPresent;
Expand Down
46 changes: 33 additions & 13 deletions Core/GameEngine/Source/GameNetwork/NetCommandWrapperList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ NetCommandWrapperListNode::NetCommandWrapperListNode(NetWrapperCommandMsg *msg)
m_chunksPresent[i] = FALSE;
}

m_dataLength = msg->getTotalDataLength();
m_data = NEW UnsignedByte[m_dataLength]; // pool[]ify
m_totalDataLength = msg->getTotalDataLength();
m_data = NEW UnsignedByte[m_totalDataLength]; // pool[]ify
Copy link
Author

@Mauller Mauller Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering if we might want to also validate the total data length, since we can't allocate MAX_UINT of memory for example in a 32bit application.

If there is not a limit considered in the game already, then we should consider something sensible for a single transfer.

Copy link

@xezon xezon Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a transfer size cap does make sense.

Copy link
Author

@Mauller Mauller Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's a matter of determining what we would consider our max, the game is still not large address aware so limited to 2/2.5GB of RAM at the moment.

Copy link

@xezon xezon Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 MB would be a reasonable limit for starters I think. But better be a separate change.

Copy link
Author

@Mauller Mauller Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, leave it for a followup.


m_commandID = msg->getWrappedCommandID();
}
Expand Down Expand Up @@ -75,7 +75,7 @@ UnsignedShort NetCommandWrapperListNode::getCommandID() {
}

UnsignedInt NetCommandWrapperListNode::getRawDataLength() {
return m_dataLength;
return m_totalDataLength;
}

void NetCommandWrapperListNode::copyChunkData(NetWrapperCommandMsg *msg) {
Expand All @@ -84,22 +84,42 @@ void NetCommandWrapperListNode::copyChunkData(NetWrapperCommandMsg *msg) {
return;
}

DEBUG_ASSERTCRASH(msg->getChunkNumber() < m_numChunks, ("MunkeeChunk %d of %d",
msg->getChunkNumber(), m_numChunks));
if (msg->getChunkNumber() >= m_numChunks)
UnsignedInt chunkNumber = msg->getChunkNumber();

if (chunkNumber >= m_numChunks) {
DEBUG_CRASH(("Data chunk %u exceeds the expected maximum of %u chunks", chunkNumber, m_numChunks));
return;
}

DEBUG_LOG(("NetCommandWrapperListNode::copyChunkData() - copying chunk %d",
msg->getChunkNumber()));
// we already received this chunk, no need to recopy it.
if (m_chunksPresent[chunkNumber] == TRUE) {
return;
}

if (m_chunksPresent[msg->getChunkNumber()] == TRUE) {
// we already received this chunk, no need to recopy it.
UnsignedInt chunkDataOffset = msg->getDataOffset();
UnsignedInt chunkDataLength = msg->getDataLength();

// TheSuperHackers @security Mauller 04/12/2025 Prevent out of bounds memory access
if (chunkDataOffset >= m_totalDataLength) {
DEBUG_CRASH(("Data chunk offset %u exceeds the total data length %u", chunkDataOffset, m_totalDataLength));
return;
}

m_chunksPresent[msg->getChunkNumber()] = TRUE;
UnsignedInt offset = msg->getDataOffset();
memcpy(m_data + offset, msg->getData(), msg->getDataLength());
if (chunkDataLength > MAX_PACKET_SIZE ) {
DEBUG_CRASH(("Data Chunk size %u greater than max packet size %u", chunkDataLength, MAX_PACKET_SIZE));
return;
}

if (chunkDataOffset + chunkDataLength > m_totalDataLength) {
DEBUG_CRASH(("Data chunk exceeds data array size"));
return;
}

DEBUG_LOG(("NetCommandWrapperListNode::copyChunkData() - copying chunk %u", chunkNumber));

memcpy(m_data + chunkDataOffset, msg->getData(), chunkDataLength);

m_chunksPresent[chunkNumber] = TRUE;
++m_numChunksPresent;
}

Expand Down
Loading