Skip to content

Add support for continuation frame in WebSocketMessageParser #2320

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
an-tao merged 4 commits into from
Jun 4, 2025
+36 −13
Merged

Add support for continuation frame in WebSocketMessageParser #2320

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
576a78f
Keep parsing if FIN was not reached but bytes are available still
dm385 May 23, 2025
2472bb5
Use loops instead of recursion
an-tao May 23, 2025
0bc4b22
Fix
an-tao May 23, 2025
42cc366
Fix infinite loop
an-tao May 24, 2025
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
49 changes: 36 additions & 13 deletions lib/src/WebSocketConnectionImpl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <json/value.h>
#include <json/writer.h>
#include <thread>
#include <limits>

using namespace drogon;

Expand Down Expand Up @@ -268,14 +269,14 @@ bool WebSocketMessageParser::parse(trantor::MsgBuffer *buffer)
{
// According to the rfc6455
gotAll_ = false;
if (buffer->readableBytes() >= 2)
while (buffer->readableBytes() >= 2)
Copy link

Copilot AI May 25, 2025

Choose a reason for hiding this comment

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

Consider adding a comment to clarify that the while loop is intentionally used to process multiple frames in a single parse call, ensuring that continuation frames are handled correctly.

Copilot uses AI. Check for mistakes.
{
unsigned char opcode = (*buffer)[0] & 0x0f;
bool isControlFrame = false;
switch (opcode)
{
case 0:
// continuation frame
LOG_TRACE << "continuation frame";
break;
case 1:
type_ = WebSocketMessageType::Text;
Expand Down Expand Up @@ -327,8 +328,13 @@ bool WebSocketMessageParser::parse(trantor::MsgBuffer *buffer)
{
indexFirstMask = 10;
}
if (indexFirstMask > 2 && buffer->readableBytes() >= indexFirstMask)
if (indexFirstMask > 2)
{
if (buffer->readableBytes() < indexFirstMask)
{
// Not enough data yet, wait for more.
return true;
}
if (isControlFrame)
{
// rfc6455-5.5
Expand All @@ -344,14 +350,17 @@ bool WebSocketMessageParser::parse(trantor::MsgBuffer *buffer)
}
else if (indexFirstMask == 10)
{
length = (unsigned char)(*buffer)[2];
length = (length << 8) + (unsigned char)(*buffer)[3];
length = (length << 8) + (unsigned char)(*buffer)[4];
length = (length << 8) + (unsigned char)(*buffer)[5];
length = (length << 8) + (unsigned char)(*buffer)[6];
length = (length << 8) + (unsigned char)(*buffer)[7];
length = (length << 8) + (unsigned char)(*buffer)[8];
length = (length << 8) + (unsigned char)(*buffer)[9];
length = 0;
for (int i = 2; i <= 9; ++i)
{
if (length > ((std::numeric_limits<size_t>::max)() >> 8))
Copy link

Copilot AI May 25, 2025

Choose a reason for hiding this comment

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

Add a comment clarifying that this check prevents an overflow when accumulating the extended payload length, ensuring safe processing of large payloads.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@marty1885 marty1885 May 27, 2025

Choose a reason for hiding this comment

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

This does not feel right. size_t is different on 32 and 64bit systems. So the max size will be 24 and 48bits respectively. Do you intend to use uint64_t?

Copy link
Copy Markdown
Member

@an-tao an-tao May 27, 2025

Choose a reason for hiding this comment

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

The judgment is followed by an 8-bit left shift operation, so the maximum values ​​are 32-bit and 64-bit respectively. I think this judgment protects the 32-bit system from overflow.

Copy link
Copy Markdown
Member

@marty1885 marty1885 May 27, 2025

Choose a reason for hiding this comment

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

On 32bit system this is 16M. Which is not big in modern standards. Is this really ok? And on 64bits 2^56 is, a lot. I think we should have a consistent limit instead of "it depends on the processor"

{
LOG_ERROR
<< "Payload length too large to handle safely";
return false;
}
length = (length << 8) + (unsigned char)(*buffer)[i];
}
}
else
{
Expand Down Expand Up @@ -380,9 +389,16 @@ bool WebSocketMessageParser::parse(trantor::MsgBuffer *buffer)
{
message_[oldLen + i] = (rawData[i] ^ masks[i % 4]);
}
Copy link

Copilot AI May 25, 2025

Choose a reason for hiding this comment

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

[nitpick] Review whether consuming the entire frame, regardless of the FIN flag, is the intended design for continuation frames. A brief inline comment explaining this decision may improve clarity for future maintainers.

Suggested change
}
}
// Consume the entire frame from the buffer, regardless of the FIN flag.
// This is necessary to process subsequent frames correctly, as the WebSocket
// protocol allows fragmented messages. The `message_` buffer accumulates
// data from all frames, and the `isFin` flag determines when the message
// is complete.

Copilot uses AI. Check for mistakes.
buffer->retrieve(indexFirstMask + 4 + length);
if (isFin)
{
gotAll_ = true;
buffer->retrieve(indexFirstMask + 4 + length);
return true;
}
}
else
{
// Not enough data yet, wait for more.
return true;
}
}
Expand All @@ -392,9 +408,16 @@ bool WebSocketMessageParser::parse(trantor::MsgBuffer *buffer)
{
auto rawData = buffer->peek() + indexFirstMask;
message_.append(rawData, length);
buffer->retrieve(indexFirstMask + length);
if (isFin)
{
gotAll_ = true;
buffer->retrieve(indexFirstMask + length);
return true;
}
}
else
{
// Not enough data yet, wait for more.
return true;
}
}
Expand Down
Loading