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.

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

[server endpoint] packet too large to send. packet is 8330 bytes, maximum is 8192 #78

Closed
maxw opened this Issue Jan 20, 2018 · 28 comments

Comments

Projects
None yet
2 participants
@maxw
Contributor

maxw commented Jan 20, 2018

I spent a few hours digging through reliable.io and yojimbo source code, and I can't seem to find the cause of this error: [server endpoint] packet too large to send. packet is 8330 bytes, maximum is 8192

As far as I can tell, yojimbo has a buffer for a single packet. When SendPackets() is called, it loops through each channel and asks it to fill the buffer up with as many messages as it can without going over the packet size limit.

However, reliable.io seems to think it's still going over. The few times I've been able to trigger this error, it's only ever over by ~150 bytes. That seems too high for something like the header size not being taken into account, but I can't figure out what's causing it exactly.

Any ideas?

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Feb 11, 2018

Can you please get me a simple example program that reproduces this error? Thanks!

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 6, 2018

Will have some time to finally debug this over the weekend. Will keep you posted.

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 8, 2018

Made some progress here. At a high level, GeneratePacket creates a ConnectionPacket that's too large. It's passed to WritePacket, which creates a WriteStream/BitWriter with the maxPacketSize set as the limit. My server was built for release, so the asserts in WriteStream/BitWriter that check the limit were stripped which is why it was getting all the way to reliable.io before getting caught.

It's worth noting that without these checks in production, this is a pretty nasty buffer overflow in BitWriter. I'd like to modify yojimbo to perform these bounds checks in both debug and release builds rather than using yojimbo_assert. Let me know if you're cool with that and I'll make the change.

Anyway, diving in it looks like SerializeBlockFragment is the call that leads to the serialize_bytes()/WriteStream.SerializeBytes()/BitWriter.WriteBytes() call that pushes BitWriter past the end of its buffer. That said, I don't believe it's an issue with SerializeBlockFragment itself.

My best guess right now is that one of my custom messages causes MeasureStream and WriteStream to serialize a different number of bytes for each pass, but I haven't been able to track down exactly how GeneratePacket() prevents itself from creating a ConnectionPacket that's too large to serialize into a single packet.

Anyway, I'll keep digging tomorrow. Let me know if you've got any insight that will help me track it down and make a fix.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 8, 2018

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 8, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 8, 2018

I'm just using the defaults right now. Here's the function that generates the config for both client and server:

Yojimbo::ClientServerConfig CommonAdapter::GetClientServerConfig() {
	Yojimbo::ClientServerConfig config;
	config.protocolId = 0;

	// Configure channels
	config.numChannels = NumberOfRealtimeChannels;
	config.channel[UnreliableChannel].type = Yojimbo::CHANNEL_TYPE_UNRELIABLE_UNORDERED;
	config.channel[ReliableChannel].type   = Yojimbo::CHANNEL_TYPE_RELIABLE_ORDERED;
	config.channel[AudioChannel].type      = Yojimbo::CHANNEL_TYPE_UNRELIABLE_UNORDERED;

#ifdef NDEBUG
	config.networkSimulator = false;
#endif

	return config;
}

Max

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 8, 2018

Worth noting that all of my packets are below 1/4th the default max packet size (8192). I have one message type of variable length, but it's only created if the data is less than 1/4th the max packet size. If the data is greater, it switches over to using a block message type.

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 8, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 8, 2018

Haha, that'll do it! Is there a sample that does this? I based this config solely on the client/server samples provided.

Are you cool with me adding bounds checks to BitWriter when compiled for release? I'm guessing this config change will fix things, but I'd still rather have the server throw an exception before allowing a buffer overflow.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 8, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 9, 2018

I'll aim for that. I just want some sort of overflow protection for security.

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 11, 2018

I've set packet budgets, but even with this configuration BitWriter is still pushed over the maxPacketSize limit:

Yojimbo::ClientServerConfig CommonAdapter::GetClientServerConfig() {
	Yojimbo::ClientServerConfig config;
	config.protocolId = 0;

	// Configure channels
	config.numChannels = NumberOfRealtimeChannels;
	config.channel[0].type   = Yojimbo::CHANNEL_TYPE_UNRELIABLE_ORDERED;
        config.channel[0].packetBudget = 7*1024; // maxPacketSize is 8*1024
	config.channel[1].type = Yojimbo::CHANNEL_TYPE_RELIABLE_UNORDERED;
        config.channel[1].packetBudget = -1;

#ifdef NDEBUG
	config.networkSimulator = false;
#endif

	return config;
}

From looking at the implementation, yojimbo calculates availableBits to determine how much room is left to write in the packet. Without budgets, it goes over this limit. With budgets, it will still go over this limit. Setting a packetBudget decreases the value in availableBits, but that doesn't solve the problem that yojimbo is miscalculating how many messages it can fit in that budget.

I'm out for a few days, but will try debugging this some more when I get back.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 11, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 12, 2018

Still working on a reproducible test case, but I do believe I've found the source of the bug: GeneratePacket() calls GetPacketData() on each channel, which returns how many bits that channel used or zero if it's unable to fit anything inside of availableBits worth of space. GeneratePacket() subtracts the number of bits that channel claims to have used from availableBits and moves onto the next channel.

The issue is that ReliableOrderedChannel::GetPacketData() ignores availableBits if the channel is serializing a block message. Therefore, if a previous channel has filled up most of the packet and availableBits isn't large enough to hold a block fragment, GetPacketData() queues the block fragment anyway and returns the number of bits used, which ends up being larger than availableBits which becomes negative.

At this point WritePacket() is called with a packet that will overflow the buffer supplied to BitWriter.

Max

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 13, 2018

Ok I've got it down to a small reproducible case that works every time.

Open up the client_server.cpp example and replace the config with this one:

ClientServerConfig config;
config.numChannels = 2;
config.channel[0].type = CHANNEL_TYPE_UNRELIABLE_UNORDERED;
config.channel[0].packetBudget = 8000; // Large enough that after this channel fills this budget, the amount of space left in the packet isn't large enough for a reliable block fragment.
config.channel[1].type = CHANNEL_TYPE_RELIABLE_ORDERED;
config.channel[1].packetBudget = -1;

Add this to the end of the while loop after yojimbo_sleep( deltaTime );

// The max packet size is 8192. Fill up the packet so there's still space left, but not enough for a full reliable block fragment.
TestBlockMessage *message = (TestBlockMessage *)client.CreateMessage(TEST_BLOCK_MESSAGE);
uint8_t *blockData = client.AllocateBlock(7169);
client.AttachBlockToMessage(message, blockData, 7169);
client.SendMessage(0, message); // Unreliable channel

// Send a block message on the reliable channel. The message will be split into 1024 byte fragments. The first fragment will attempt to write beyond the end of the packet buffer and crash.
message = (TestBlockMessage *)client.CreateMessage(TEST_BLOCK_MESSAGE);
blockData = client.AllocateBlock(1024);
client.AttachBlockToMessage(message, blockData, 1024);
client.SendMessage(1, message); // Reliable channel

This will assert in debug mode and overflow when compiled for release mode.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 13, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 13, 2018

Yeah totally.

One question on that. Here's the block that needs the check: https://github.com/networkprotocol/yojimbo/blob/master/yojimbo.cpp#L1799

Can I just add a check if fragmentBits <= availableBits ? or is the fragment already removed from a queue after calling GetFragmentToSend()/GetFragmentPacketData()? I get the feeling that if I don't use the fragment after calling those functions, it will be dropped, but I'm not 100% sure.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 13, 2018

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 13, 2018

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 13, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 16, 2018

I've made a test case that works and have started working on the bug. It looks like GetFragmentToSend() marks the block as active and does a few other things that I'd imagine shouldn't be performed until we know we're going to send a fragment for sure.

I'm thinking the best guess is going to be a check that occurs before GetFragmentToSend() is even called. Would something like this make sense?

if ( SendingBlockMessage() )
{
	if (m_config.blockFragmentSize * 8 > availableBits)
		return 0;
// ...

The only issue here is that if the final fragment is smaller than the blockFragmentSize (which it almost always will be), this will stall until there's blockFragmentSize worth of space in the packet. I can't seem to figure out a nice way to get the actual size of the next fragment without changing a ton of state inside of yojimbo that assumes the fragment will be sent.

Would love some direction on this one.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 17, 2018

I can't seem to figure out a nice way to get the actual size of the next fragment without changing a ton of state inside of yojimbo that assumes the fragment will be sent.

Fragments are always a fixed size as defined by the channel config.

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 17, 2018

Oh I see, the final fragment. Well, that was an, admittedly dumb optimization of mine.

Why not just make it simpler, and for the final fragment, just send a whole sized fragment. This way, the calculations become simple.

What a pointless micro-optimization... what was I thinking when I wrote this...

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 25, 2018

I've implemented a fix as you describe. However, now that I think about it, it's not just a micro-optimization for the final fragment, but any block message that's less than 1024 bytes.

The fix I've implemented will never send a block message that's smaller than 1024 bytes until there's at least 1024 bytes available for that channel, which I imagine could be a big issue for some people. (esp for games that want to fill the packet with mostly unreliable messages, and an occasional reliable rpc here and there)

If you can give me some info on how to get the current fragment size w/o calling GetFragmentToSend(), I'd love to update this fix to account for that edge-case.

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 25, 2018

@maxw

This comment has been minimized.

Contributor

maxw commented Apr 26, 2018

sorry by 1024 I mean config.blockFragmentSize which defaults to 1024.

Max

@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 26, 2018

gafferongames added a commit that referenced this issue Apr 26, 2018

Merge pull request #87 from NormalVR/bug/ReliableFragmentOverflowBug
Implement fix (and test case) for reliable fragment overflow bug (Issue #78)
@gafferongames

This comment has been minimized.

Member

gafferongames commented Apr 26, 2018

This is fixed now.

cbiasco added a commit to Isetta-Team/yojimbo that referenced this issue Sep 14, 2018

Merge pull request networkprotocol#87 from NormalVR/bug/ReliableFragm…
…entOverflowBug

Implement fix (and test case) for reliable fragment overflow bug (Issue networkprotocol#78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment