fix(proto): do not coalesce into a too small datagram tail

[HeikoBornholdt]

Description

Fixes #791.

The CONNECTION_CLOSE branch passes its datagram to the next packet number space without checking that there is room for another packet. With initial_mtu above 1200 the Initial CONNECTION_CLOSE packet, padded to 1200, leaves a tail behind, and when that tail is under MIN_PACKET_SPACE the Handshake space writes into it anyway.

In a release build, where the assert is gone, it goes ahead and does that. At min_mtu and initial_mtu of 1250, poll_transmit hands back size = 1282, segment_size = 1250. The Handshake packet starts at byte 1200 and ends at 1282, so it runs 32 bytes past the end of the datagram it went into. GSO then sends two datagrams, 1250 and 32 bytes, and the Handshake CONNECTION_CLOSE packet is cut in half between them. Neither half can be decrypted, so the other side drops both, and those last 32 bytes go out as their own useless datagram. My connection still closed, but only because the Initial copy of the CONNECTION_CLOSE is intact in the first datagram.

For the fix I did not add a second size check to the CONNECTION_CLOSE branch. Instead I made the loop keep the promise the assert makes: if what is left of the datagram cannot hold a packet, finish the datagram, so the code below starts a new one for the next packet. TransmitBuf::finish_datagram does that. For the first datagram of a batch it is clip_segment_size; a later one ends up shorter than the segment size, so the batch has to end there and max_datagrams is capped.

It only does anything in the cases the assert rejects today, so anything that passes the assert works exactly as before.

Breaking Changes

None 🤞.

Change checklist

• Self-review.
• Tests if relevant.
• This PR was created by a human that thought critically about the
  proposed change and wrote an as clear and concise description as
  they could.
• This PR isn't slop, and is carefully crafted to do have the
  intented effect.
• cargo make passes locally.

[flub]

Thanks for the great bug report! Interesting to see you hit this edge case, and apologies for leaving it unresolved with just a TODO. There's a bunch more improvments we need to make to this code!

I think your bug report points to the padding being wrong after the CONNECTION_CLOSE was written in the fist space.

When PacketBuilder::finish_and_track is called with PadDatagram::ToMinMtu it should not use MIN_INITIAL_SIZE, but should rather use the configured minimum MTU. Possibly all uses of MIN_INITIAL_SIZE are wrong? Anyway, in this case maybe the TransmitBuf can gain a field to know what the min_mtu is so that it can do the padding correctly?

Secondly as you say the check for whether enough space is remaining to coalesce is missing in the connection-close branch. I'd expect to check this before the builder.finish_and_track call on line 1704 so that it passes in the correct amount of padding to builder.finish_and_track (i.e. 0 or min_mtu).

I think those two changes together would also fix the issue you experience? Does this make sense?

[HeikoBornholdt]

Thank you! Also for taking a look.

I tried the padding change and I don't think it closes the hole. With min_mtu at 1200 and initial_mtu at 1250 the assert fires exactly as before. The tail is the datagram size minus whatever we padded to, and the datagram size is the path MTU, so for any padding size P the window from P to P + MIN_PACKET_SPACE stays broken: today that's an MTU of 1201 to 1286, with the change it would be initial_mtu sitting 1 to 86 bytes above min_mtu. Let me know if I'm missing something.

It did show up a weakness in my test though, which had both values at 1250. I've changed it to min_mtu at 1200 and only initial_mtu raised, which leaves the 50 byte tail. The difference has to stay under MIN_PACKET_SPACE, otherwise the tail holds a whole packet and there's nothing to trip over.

The other uses of MIN_INITIAL_SIZE all look like the RFC minimum to me, including the inbound check in endpoint.rs, which is about the peer's padding rather than ours, so I've left the constant as it is.

On the space check, agreed. I put it at the top of the space loop rather than before finish_and_track because there the leftover is already known instead of having to be worked out before the packet is finished, and because it then holds for any space that hands its datagram on, not just the close branch. A tail too small to use becomes "no datagram", so the code below starts a new one. If you'd rather have it where you suggested, happy to move it, the check itself is the same either way.

I also shortened your TODO above the branch to the parts that are still open, easy to put back.

[flub]

Hum yes, my comment about padding doesn't make much sense. Also good catch on min_mtu vs initial_mtu.

IIUC when this condition hits there will be a CONNECTION_CLOSE frame missing in one of the higher spaces? That is still a remaining bug, but a lot less harmful. It's fine to not address in this PR, small steps are great, I only want to understand the implications.

[HeikoBornholdt]

You're right, CONNECTION_CLOSE will be missing.
Note: this is only the case when GSO is not used.