Skip to content
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

[FIX] Calculate correctly max payload size per UDP packet #2677

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Mar 1, 2023

Fixes #2663

This fixes the problem of specifying the maximum payload size per packet. Some of the internal functions have also slightly changed their meaning and logics.

The problem: the system header (IP and UDP part) has a total length dependent on the IP version, and so it affects the remaining space in the 1500 MTU. For IPv4 it splits to 20[IPv4] + 8[UDP] + 16[SRT] + 1456. For IPv6, 32[IPv6] + 8[UDP] + 16[SRT] + 1444. Therefore the constants that were introduced for the maximum payload size have been deprecated and new ones were set up.

The real problem is actually that it depends on the effective IP version that is known only after the connection has been established - even if you declare the address in srt_bind or srt_connect to be IPv6, if the peer is IPv4, the in-transmission packet layout is like for IPv4. The actual rules of which layout to use are:

  • For IPv4 socket, use always IPv4 layout
  • For IPv6 socket, depending on the peer's address:
    • if the peer address is IPv6-mapped-IPv4 address, use the layout for IPv4
    • otherwise use the layout for IPv6

In other words, it's not enough to state that the current socket is IPv6, but you need to wait for a connection because if the connection is made to an IPv4 address, the transmission will use IPv4, and the IPv6 socket is just the matter of the agent's system API (it uses dual-stack sockets in reality). As confirmed in experiments, there isn't any limitation in the API how big packet you may send (except that for 65535 bytes) - if it exceeds the payload size for the UDP with particular IP version, worst case the packet will be fragmented and reassembled on the peer side transparently. The problem is then only in the following range:

  • In live mode, if we send a packet, we should consider having sent exactly one packet, without initial fragmentation

  • In file mode, if we split a bigger buffer into smaller units that will be sent in single UDP packets, it would be counter-productive if a packet happens to have 1456 payload added with 40 bytes header and get effectively split into one unit of 40+16+1444 bytes and one of 40+16+12 bytes.

Fixes embrace also all internal calculations:

  1. The SRTO_PAYLOADSIZE option: this is used only in live mode and defines the maximum size of a buffer specified for sending in one srt_send call, as well as defines the maximum size used for data in the buffers, and calculations of data stats. This option has also changed meaning on reading: for a connected socket it has a fixed value and returns the current maximum payload per packet in all modes. In file mode, where this option defaults to 0, after connecting (when this option can't be any longer altered) it returns the value of the maximum payload size per packet that will be used - that is, in live mode it will return the earlier set value (as long as it was accepted), while in file mode it will return 1456 or 1444 (with no AEAD, no FEC and default value of MSS), depending on which IP protocol is in use. This value is also additionally controlled with regard to this, however it can't really control this option when the underlying protocol is unknown, so setting a too big value for this option with regard to IPv6 may result in error only during the call to srt_bind or srt_connect.
  2. Allocation in the receiver queue and both buffers: The sender buffer is created only at the connection time, so it is known only then what maximum payload size should be used and what the header size is (required for rate estimator). However the receiver queue (unit queue) that is used by the receiver buffer must allocate the maximum payload size because the actual interchange IP protocol isn't known at the moment if creation. Therefore the receiver buffer's unit queue is allocated with every unit having the payload size equal to 1456.

srtcore/api.cpp Outdated

// We can't use maxPayloadSize() because this value isn't valid until the connection is established.
// We need to "think big", that is, allocate a size that would fit both IPv4 and IPv6.
size_t payload_size = s->core().m_config.iMSS - CPacket::HDR_SIZE - CPacket::udpHeaderSize(AF_INET);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a controversy whether this size should be used, or rather the payload size set by an option, if it's less, to allocate less memory. I think this shouldn't be done because if you happen to receive an SRT packet that would be oversized in these conditions, it will end up in a call that results in getting MSG_TRUNC flag due to too small buffer for the packet, which may lead to hard to detect errors.

srtcore/core.cpp Outdated
// IMPORTANT:
// The m_iMaxSRTPayloadSize is the size of the payload in the "SRT packet" that can be sent
// over the current connection - which means that if any party is IPv6, then the maximum size
// is the one for IPv6 (1444). Only if both parties are IPv4, this maximum size is 1456.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment is likely wrong. 1444 is only when both parties are confirmed as IPv6.

srtcore/srt.h Dismissed Show dismissed Hide dismissed
srtcore/stats.h Fixed Show fixed Hide fixed
@ethouris ethouris marked this pull request as ready for review March 3, 2023 17:08
@ethouris ethouris marked this pull request as draft March 6, 2023 13:08
@ethouris ethouris marked this pull request as ready for review March 7, 2023 16:47
Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Very minor edits.

@@ -1204,6 +1204,7 @@ class CUDT
sockaddr_any m_PeerAddr; // peer address
sockaddr_any m_SourceAddr; // override UDP source address with this one when sending
uint32_t m_piSelfIP[4]; // local UDP IP address
int m_TransferIPVersion; // AF_INET/6 that should be used to determine common payload size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int m_TransferIPVersion; // AF_INET/6 that should be used to determine common payload size
int m_iTransferIPVersion; // AF_INET/6 that should be used to determine the common payload size

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually not. The i marker defines an integer - counting - variable, while here it is used as a value that should be assigned to AF_INET or AF_INET6 symbol. If this was a decent C++ API, it would be an enum.

srtcore/srt.h Outdated
Comment on lines 305 to 316

// These constants define the maximum size of the payload
// in a single UDP packet, depending on the IP version, and
// with the default socket options, that is:
// * default 1500 bytes of MTU (see SRTO_MSS)
// * without FEC packet filter (see SRTO_PACKETFILTER)
// * without AEAD through AES-GCM (see SRTO_CRYPTOMODE)
static const int SRT_MAX_PLSIZE_AF_INET = 1456; // MTU(1500) - IPv4.hdr(20) - UDP.hdr(8) - SRT.hdr(16)
static const int SRT_MAX_PLSIZE_AF_INET6 = 1444; // MTU(1500) - IPv6.hdr(32) - UDP.hdr(8) - SRT.hdr(16)

// A macro for these above in case when the IP family is passed as a runtime value.
#define SRT_MAX_PLSIZE(famspec) ((famspec) == AF_INET ? SRT_MAX_PLSIZE_AF_INET : SRT_MAX_PLSIZE_AF_INET6)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am hesitant in having these in the API, because the maximum payload size also depends on the MTU size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was simply trying to do some replacement for the existing the "MAX PLSIZE" constant. Not sure if anyone is using it or whether it is anyhow useful. Note that the live payload size (1316) is still lower than these two, also including FEC and AEAD. So the only use case for these constants is for a case when you have a live stream, but you want to have a bigger payload than 1316. It is also clearly written here, under which circumstances these values hold.

We can remove this, but note that the true maximum payload size can only be determined after connection. As an alternative, we can have some tools to determine the payload size with foreseen IP version, AEAD and FEC.

srtcore/srt.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Payload size rules are tied to IPv4 only
4 participants