diff --git a/include/AlcPacket.h b/include/AlcPacket.h index cce7ff5..392ce6e 100644 --- a/include/AlcPacket.h +++ b/include/AlcPacket.h @@ -68,7 +68,8 @@ namespace LibFlute { AlcPacket(uint64_t tsi, CloseSession); AlcPacket(uint64_t tsi, uint16_t toi, FecOti fec_oti, const std::vector& symbols, size_t max_size, uint32_t fdt_instance_id, - bool close_session_flag = false, bool close_object_flag = false); + bool close_session_flag = false, bool close_object_flag = false, + bool include_fti = false); /** * Default destructor. diff --git a/include/File.h b/include/File.h index de3a7f9..e566de7 100644 --- a/include/File.h +++ b/include/File.h @@ -129,6 +129,16 @@ namespace LibFlute { _meta.content_type = fdt_entry.content_type; _meta.content_md5 = fdt_entry.content_md5; _meta.expires = fdt_entry.expires; + // The encoding has to come across too. Reception bootstrapped from EXT_FTI cannot know an + // object is content encoded, because EXT_FTI carries FEC parameters and not that; without + // this the object is delivered still compressed, with a Content-MD5 that will not match. + _meta.content_encoding = fdt_entry.content_encoding; + // And so does the content length. Bootstrapping sets it from EXT_FTI's transfer length, + // which is the same number only for an object carried without a content encoding. For an + // encoded one the FDT holds the decoded length and EXT_FTI the encoded one, so keeping the + // bootstrap value makes the post-decode length check compare against the wrong figure. + // fec_oti is still left alone: its transfer length is what the reassembly is keyed on. + _meta.content_length = fdt_entry.content_length; }; /** diff --git a/include/Receiver.h b/include/Receiver.h index 0fd8f1b..6e19d8d 100644 --- a/include/Receiver.h +++ b/include/Receiver.h @@ -18,9 +18,12 @@ #include #include #include +#include #include #include #include +#include +#include #include "File.h" #include "FileDeliveryTable.h" @@ -48,6 +51,44 @@ namespace LibFlute { * @param toi The TOI the packet carrying the flag(s) was for */ typedef std::function close_notification_callback_t; + + /** + * Definition of the caller-supplied hook that locates the ALC/LCT payload inside a + * tunnelled datagram. + * + * https://github.com/5G-MAG/rt-libflute/issues/66 : the Receiver-side counterpart to + * Transmitter's existing udp_tunnel_address() support (see its _tunnel_endpoint / + * create_ip_hdr() / create_udp_pkt()). Per the issue discussion: the library has no + * business knowing about any particular encapsulation format -- that is entirely the + * controlling application's concern ("a better design pattern would be for the + * controlling application to pass in a 'helper' function that the library invokes to + * do application-specific mangling of packets before the generic code in the library + * starts processing the ALC/LCT payload", rjb1000). This is that helper's signature. + * + * This mirrors the de-tunnelling logic that already exists, hand-written, in + * tests/test_end_to_end.cpp's run_tunnel_bridge() (added alongside Transmitter's own + * tunnel mode) -- a std::thread there receives on a plain UDP socket, manually parses + * a hand-built inner IPv4+UDP header out of the payload (no GTP-U or other framing; + * Transmitter's tunnel mode wraps the ALC packet in exactly this and nothing else), + * and forwards just the FLUTE bytes onward over loopback to a receiver that has no + * tunnel-awareness at all. This issue asks for that same logic to move inside Receiver + * proper, as a caller-supplied, protocol-agnostic hook rather than a hardcoded parser + * baked into the library, so any encapsulation a given deployment actually needs + * (matching Transmitter's own wrapper, real GTP-U, or anything else) is expressed + * purely by what modifier the caller passes in. + * + * @param payload The whole datagram as received on the tunnel socket. The callback may + * freely inspect and/or edit its contents in place (e.g. to decrypt a payload + * that arrives encrypted under the encapsulation, not just to locate it). + * @return The byte offset within (the, possibly now-modified) @p payload at which the + * ALC/LCT payload begins. A return value >= payload.size() (e.g. SIZE_MAX) + * tells the Receiver to silently discard this datagram without attempting ALC + * parsing -- there is no separate bool/optional discard signal; encoding + * "nothing usable here" as "there are no bytes left to read" keeps this one + * consistent, easy-to-satisfy contract rather than two. + */ + typedef std::function& payload)> packet_modifier_t; + /** * Default constructor. * @@ -58,11 +99,45 @@ namespace LibFlute { * @param io_context Boost io_context to run the socket operations in (must be provided by the caller) * @param source_address If non-empty, join as source-specific multicast (SSM, IPv4 * only) admitting only packets from this source -- otherwise ASM (any-source). + * Unrelated to the tunnel parameters below; this is the pre-existing plain + * multicast join, unchanged. + * @param tunnel_address If given, ALSO bind a plain unicast UDP socket to this local + * endpoint and accept tunnelled datagrams there, in addition to the normal + * multicast join above. Motivating cases (see the issue): a deployment that + * needs to receive FLUTE content arriving encapsulated (e.g. GTP-U) rather than + * as plain IP multicast, or one where the platform's local multicast delivery + * isn't available on the path content actually arrives over at all (confirmed + * live: a datagram written to a software TUN device, as a UE simulator's own + * decapsulated-content path does, never reaches a socket joined to its + * destination multicast group on that interface -- the platform's multicast + * delivery never triggers for it at all -- even though the identical datagram + * delivers correctly on a real network interface). The two paths are + * independent and both feed the same session state, so either one arriving is + * enough; this is deliberately not an either/or choice like Transmitter's + * tunnel mode, since a Receiver has no way to know in advance which path will + * actually work in a given deployment. + * @param tunnel_source If given, only accept tunnel datagrams whose UDP source address + * matches this value -- the tunnel-socket equivalent of @p source_address's SSM + * admit-only-this-source semantics, since the tunnel socket itself is a plain + * unicast bind with no multicast-layer source filtering of its own. This is + * the "extra address checking" the library itself does, on top of whatever + * @p packet_modifier does -- source-address admission is a generic, + * encapsulation-agnostic concept the library can reasonably own, unlike parsing + * any particular header format. + * @param packet_modifier Required whenever tunnel_address is set (ignored otherwise, + * and if omitted while tunnel_address is set, every tunnel datagram is + * discarded -- silently failing open would be far worse than silently + * discarding). See packet_modifier_t; there is no default implementation, since + * any default would itself bake an assumption about the tunnel's wire format + * into the library, exactly what this issue asks not to do. */ Receiver( const std::string& iface, const std::string& address, short port, uint64_t tsi, boost::asio::io_context& io_context, - const std::string& source_address = ""); + const std::string& source_address = "", + const std::optional& tunnel_address = std::nullopt, + const std::optional& tunnel_source = std::nullopt, + const std::optional& packet_modifier = std::nullopt); /** * Destructor. Marks the receiver as no longer alive so that any async_receive_from @@ -120,9 +195,28 @@ namespace LibFlute { void handle_receive_from(const boost::system::error_code& error, size_t bytes_recvd); void arm_receive(); + // The actual ALC/FLUTE processing, shared by both the normal multicast-socket path + // (handle_receive_from(), data already at offset 0 in _data) and the tunnel path + // (handle_tunnel_receive_from(), data at whatever offset packet_modifier_t returns + // inside _tunnel_data) -- see packet_modifier_t's comment in the header for why these + // are two independent, simultaneously-armed receive loops rather than a single one. + void process_alc_datagram(char* data, size_t len); + void handle_tunnel_receive_from(const boost::system::error_code& error, + size_t bytes_recvd); + void arm_tunnel_receive(); boost::asio::ip::udp::socket _socket; boost::asio::ip::udp::endpoint _sender_endpoint; + std::unique_ptr _tunnel_socket; + boost::asio::ip::udp::endpoint _tunnel_sender_endpoint; + std::optional _tunnel_source; + packet_modifier_t _packet_modifier; + // Sized like _data (see max_length below) but as a resizable buffer rather than a + // fixed char array: packet_modifier_t takes a std::vector& so a caller-supplied + // modifier can shrink/grow the datagram in place (e.g. after decrypting a payload that + // changes size), not just report where to start reading a fixed buffer. + std::vector _tunnel_data; + // Must hold the largest UDP datagram that can actually arrive: with the // Compact No-Code FEC scheme, a packet is a 4-byte SBN+ID header plus one // full encoding symbol, and FEC-OTI-Encoding-Symbol-Length is a per-session diff --git a/src/AlcPacket.cpp b/src/AlcPacket.cpp index 0ab3ca6..186cd7d 100644 --- a/src/AlcPacket.cpp +++ b/src/AlcPacket.cpp @@ -240,7 +240,8 @@ LibFlute::AlcPacket::AlcPacket(char* data, size_t len) } LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_oti, const std::vector& symbols, size_t max_encoding_symbol_size, uint32_t fdt_instance_id, - bool close_session_flag, bool close_object_flag) + bool close_session_flag, bool close_object_flag, + bool include_fti) : _fec_oti(fec_oti) { // TSI width: this wire scheme always carries a 16-bit half-word component (half_word_flag=1, @@ -258,8 +259,10 @@ LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_ if (wide_tsi) { lct_header_len += 1; } - if (toi == 0) { // Add extensions for FDT + if (toi == 0) { // EXT_FDT (one word) plus EXT_FTI (four words) lct_header_len += 5; + } else if (include_fti) { // EXT_FTI only, four words + lct_header_len += 4; } auto max_packet_length = max_encoding_symbol_size + @@ -295,14 +298,27 @@ LibFlute::AlcPacket::AlcPacket(uint64_t tsi, uint16_t toi, LibFlute::FecOti fec_ *((uint16_t*)hdr_ptr) = htons(toi); hdr_ptr += 2; - if (toi == 0) { // Add extensions for FDT + if (toi == 0) { // EXT_FDT describes the FDT instance and belongs only on the FDT itself *((uint8_t*)hdr_ptr) = EXT_FDT; hdr_ptr += 1; *((uint8_t*)hdr_ptr) = 1 << 4 | (fdt_instance_id & 0x000F0000) >> 16; hdr_ptr += 1; *((uint16_t*)hdr_ptr) = htons(fdt_instance_id & 0x0000FFFF); hdr_ptr += 2; + } + /* EXT_FTI goes on the FDT always, and on a content object when asked for. The caller asks when + the FDT cannot carry the object's transfer length, which under the MBMS Download Profile is any + content-encoded object: TS 26.346 V18.2.0 clause L.4.4 forbids Transfer-Length in the FDT and + RFC 3926 clause 3.4.2 only lets Content-Length stand in for an unencoded object, so in band is + the only route left and RFC 3926 clause 5 obliges every receiver to support it. + + This is a deliberate departure from a "should", taken because the alternative departs from a + "shall not". TS 26.346 V18.2.0 clause L.4.7: "FEC Object Transmission Information in FLUTE + packets which carry symbols of content files should be conveyed by the FEC-OTI parameters in + the FDT". The FDT still carries every FEC-OTI parameter the profile permits; what it cannot + carry, and what travels here instead, is the transfer length alone. */ + if (toi == 0 || include_fti) { *((uint8_t*)hdr_ptr) = EXT_FTI; hdr_ptr += 1; *((uint8_t*)hdr_ptr) = 4; // HEL diff --git a/src/File.cpp b/src/File.cpp index c114d7a..13a44c9 100644 --- a/src/File.cpp +++ b/src/File.cpp @@ -206,7 +206,19 @@ auto File::check_file_completion() -> void auto File::calculate_partitioning() -> void { - // Calculate source block partitioning (RFC5052 9.1) + /* Both divisors come from the FEC OTI and both are used as denominators below. A + default-constructed FecOti leaves them 0, which makes the first division produce inf, the + block count inf, and the block-creation loop below effectively unbounded: the object hangs + rather than reporting anything. Reachable through the public File constructors, which accept + a FecOti without inspecting it. Refused loudly instead, per RULES.md rule 12. + `code-derived, no spec claim`. */ + if (_meta.fec_oti.encoding_symbol_length == 0 || _meta.fec_oti.max_source_block_length == 0) { + throw std::runtime_error( + "FEC OTI is unusable for partitioning: encoding_symbol_length and " + "max_source_block_length must both be non-zero"); + } + + // Calculate source block partitioning (RFC5052 9.1) _nof_source_symbols = ceil((double)_meta.fec_oti.transfer_length / (double)_meta.fec_oti.encoding_symbol_length); _nof_source_blocks = ceil((double)_nof_source_symbols / (double)_meta.fec_oti.max_source_block_length); _large_source_block_length = ceil((double)_nof_source_symbols / (double)_nof_source_blocks); @@ -297,7 +309,19 @@ auto File::encode() -> void }; spdlog::debug("Compressing contents with {}", _meta.content_encoding); - if (deflateInit2(&zs, Z_DEFAULT_COMPRESSION, Z_DEFLATED, 15 | 16, 8, Z_DEFAULT_STRATEGY) == Z_OK) { + /* Select the framing from the encoding, matching inflateInit2 below. windowBits 15 is the + zlib wrapper, and adding 16 selects gzip instead. Hardcoding gzip here meant a file + declared as "deflate" was written with gzip framing, so this library could not read back + what it had just written, and the framing did not match what the declared encoding means. + + RFC 9110 clause 8.4.1.2: "The "deflate" coding is a "zlib" data format [RFC1950] + containing a "deflate" compressed data stream [RFC1951] that uses a combination of the + Lempel-Ziv (LZ77) compression algorithm and Huffman coding." + + General FLUTE only in practice: the MBMS Download Profile permits no encoding other than + gzip, so a 3GPP session never takes the deflate branch. */ + const int window_bits = 15 | ((_meta.content_encoding == "gzip") ? 16 : 0); + if (deflateInit2(&zs, Z_DEFAULT_COMPRESSION, Z_DEFLATED, window_bits, 8, Z_DEFAULT_STRATEGY) == Z_OK) { _buffer = nullptr; auto zstate = deflate(&zs, Z_FINISH); size_t last_out = 0; @@ -320,8 +344,14 @@ auto File::encode() -> void } _meta.fec_oti.transfer_length = zs.total_out; } else { - spdlog::error("Error compressing file {}: {}", _meta.toi, zs.msg); - throw zs.msg; + /* zs.msg is NULL for Z_STREAM_ERROR, so the previous form formatted a null char* through + spdlog and then threw it. Throwing a raw char* also meant only catch(const char*) + could handle it, and dereferencing the caught null would crash the handler. */ + const char *zmsg = zs.msg ? zs.msg : "no zlib message"; + spdlog::error("Error compressing file {}: {} (zlib status {})", _meta.toi, zmsg, zstate); + deflateEnd(&zs); + if (own_decomp) free(decomp_buffer); + throw std::runtime_error(std::string("Failed to compress file: ") + zmsg); } deflateEnd(&zs); @@ -354,7 +384,15 @@ auto File::decode() -> void inflateInit2(&zs, 15 | ((_meta.content_encoding == "gzip")?16:0)); _buffer = nullptr; - auto zstate = inflate(&zs, Z_FINISH); + /* Z_NO_FLUSH, not Z_FINISH. Z_FINISH promises inflate that the output buffer can hold the + whole result; with the 16384-byte staging buffer below that is only true for small + objects, and for anything larger inflate returns Z_BUF_ERROR with zs.msg left NULL. The + loop below continues on Z_OK, so it ran zero times and control fell straight into the + error branch, which then formatted that NULL pointer. Reproduced standalone: a 100,000 + byte object returned Z_BUF_ERROR immediately with total_out stuck at 16384, and the same + input with Z_NO_FLUSH reached Z_STREAM_END in six iterations with all 100,000 bytes. + `code-derived, no spec claim`. */ + auto zstate = inflate(&zs, Z_NO_FLUSH); size_t last_out = 0; while (zstate == Z_OK) { spdlog::debug("Part decompressed: {} bytes", 16384-zs.avail_out); @@ -364,7 +402,7 @@ auto File::decode() -> void _own_buffer = true; zs.avail_out = 16384; zs.next_out = decomp_buffer.get(); - zstate = inflate(&zs, Z_FINISH); + zstate = inflate(&zs, Z_NO_FLUSH); } if (zstate==Z_STREAM_END) { if (last_out != zs.total_out) { @@ -379,8 +417,13 @@ auto File::decode() -> void spdlog::error("Decompressed length does not match expected Content-Length ({} != {})", _meta.content_length, zs.total_out); } } else { - spdlog::error("Error decompressing file {}: {}", _meta.toi, zs.msg); - throw zs.msg; + /* zs.msg is NULL for several zlib statuses, so the previous form formatted a null + char* through spdlog and then threw it, which only catch(const char*) could take. */ + const char *zmsg = zs.msg ? zs.msg : "no zlib message"; + spdlog::error("Error decompressing file {}: {} (zlib status {})", _meta.toi, zmsg, zstate); + inflateEnd(&zs); + if (own_comp) free(comp_buffer); + throw std::runtime_error(std::string("Failed to decompress file: ") + zmsg); } if (own_comp) free(comp_buffer); diff --git a/src/FileDeliveryTable.cpp b/src/FileDeliveryTable.cpp index 24dbcfd..b11df0a 100644 --- a/src/FileDeliveryTable.cpp +++ b/src/FileDeliveryTable.cpp @@ -269,12 +269,36 @@ LibFlute::FileDeliveryTable::FileDeliveryTable(uint32_t instance_id, char* buffe content_length = strtoull(val->Value(), nullptr, 0); } + /* Parsed before the transfer length below, which depends on whether an encoding is applied. */ + auto content_encoding = std::string(); + val = file_ns.findAttribute(file, "Content-Encoding", fdt_ns); + if (val != nullptr) { + content_encoding = val->Value(); + } + uint32_t transfer_length = 0; + /* Content-Length is the transfer length only when the object is NOT content encoded. With an + encoding applied the two are different quantities, and using one for the other feeds the + decompressor a wrong input size. + + RFC 3926 clause 3.4.2: "If the file is not content encoded before transport (and thus the + "Content-Encoding" attribute is not used) then the transfer length is the length of the + original file, and in this case the "Content-Length" is also the transfer length." + + So the fallback is applied only in that case. When an encoding IS applied and no + Transfer-Length was carried, the transfer length is genuinely unknown from this FDT and is + left at 0 rather than guessed; the decode path then fails with a message naming the cause + instead of silently truncating its input. See the register entry on the profile conflict + this exposes. */ val = file_ns.findAttribute(file, "Transfer-Length", fdt_ns); if (val != nullptr) { transfer_length = strtoull(val->Value(), nullptr, 0); - } else { + } else if (content_encoding.empty()) { transfer_length = content_length; + } else { + transfer_length = 0; + spdlog::warn("File TOI {} is content encoded ({}) but carries no Transfer-Length; its " + "transfer length is not derivable from this FDT", toi, content_encoding); } auto content_md5 = std::string(); @@ -283,12 +307,6 @@ LibFlute::FileDeliveryTable::FileDeliveryTable(uint32_t instance_id, char* buffe content_md5 = val->Value(); } - auto content_encoding = std::string(); - val = file_ns.findAttribute(file, "Content-Encoding", fdt_ns); - if (val != nullptr) { - content_encoding = val->Value(); - } - auto content_type = std::string(); val = file_ns.findAttribute(file, "Content-Type", fdt_ns); if (val != nullptr) { @@ -582,8 +600,10 @@ auto LibFlute::FileDeliveryTable::to_string() const -> std::string { The parser below is deliberately unchanged, because the same clause's NOTE keeps this one mandatory for receivers: "With the exception of Transfer-Length, which is mandatory, these - parameters are optional to support by the FLUTE receiver." Nothing is lost on the wire either: - the receive path falls back to Content-Length when the attribute is absent. */ + parameters are optional to support by the FLUTE receiver." Nothing is lost on the wire: + RFC 3926 clause 3.4.2 lets Content-Length stand in for an object carried without a content + encoding, and this sender does not content encode under a 3GPP profile at all, because the + profile provides no carrier for the resulting length. See Transmitter::send(). */ if (!is_3gpp(_profile) && file.fec_oti.transfer_length) f->SetAttribute("Transfer-Length", file.fec_oti.transfer_length); if (!file.content_md5.empty()) f->SetAttribute("Content-MD5", file.content_md5.c_str()); diff --git a/src/Receiver.cpp b/src/Receiver.cpp index 70be42a..958612d 100644 --- a/src/Receiver.cpp +++ b/src/Receiver.cpp @@ -69,8 +69,14 @@ namespace { LibFlute::Receiver::Receiver ( const std::string& iface, const std::string& address, short port, uint64_t tsi, boost::asio::io_context& io_context, - const std::string& source_address) + const std::string& source_address, + const std::optional& tunnel_address, + const std::optional& tunnel_source, + const std::optional& packet_modifier) : _socket(io_context) + , _tunnel_source(tunnel_source) + , _packet_modifier(packet_modifier.value_or(nullptr)) + , _tunnel_data(max_length) , _tsi(tsi) , _mcast_address(address) , _mcast_port(static_cast(port)) @@ -179,6 +185,32 @@ LibFlute::Receiver::Receiver ( const std::string& iface, const std::string& addr } arm_receive(); + + // https://github.com/5G-MAG/rt-libflute/issues/66 -- see packet_modifier_t's comment + // for the full rationale. This is deliberately IN ADDITION to the multicast join above, + // not instead of it: a Receiver has no way to know in advance whether local multicast + // delivery will actually work on a given deployment's path, so both are always armed and + // whichever one actually receives something feeds the same session state. + if (tunnel_address) { + if (!packet_modifier) { + // Fail closed, not open: an unset packet_modifier with no way to locate the ALC + // payload inside an unknown encapsulation means every tunnel datagram would have to + // be discarded anyway (see packet_modifier_t) -- so skip standing up the tunnel + // socket at all and say why, rather than silently binding a socket that can never + // usefully deliver anything. + spdlog::error("Receiver: tunnel_address given without a packet_modifier -- " + "tunnel reception disabled, only the multicast path (if any) is active"); + } else { + _tunnel_socket = std::make_unique(io_context); + _tunnel_socket->open(tunnel_address->protocol()); + _tunnel_socket->set_option(boost::asio::ip::udp::socket::reuse_address(true)); + _tunnel_socket->set_option(boost::asio::socket_base::receive_buffer_size(16*1024*1024)); + _tunnel_socket->bind(*tunnel_address); + spdlog::info("Receiver: listening for tunnelled datagrams on {}:{}", + tunnel_address->address().to_string(), tunnel_address->port()); + arm_tunnel_receive(); + } + } } LibFlute::Receiver::~Receiver() @@ -206,6 +238,63 @@ namespace { } } +auto LibFlute::Receiver::arm_tunnel_receive() -> void +{ + auto alive = _alive; + _tunnel_data.resize(max_length); + _tunnel_socket->async_receive_from( + boost::asio::buffer(_tunnel_data), _tunnel_sender_endpoint, + [this, alive](const boost::system::error_code& error, size_t bytes_recvd) { + if (!*alive) return; + handle_tunnel_receive_from(error, bytes_recvd); + }); +} + +auto LibFlute::Receiver::handle_tunnel_receive_from(const boost::system::error_code& error, + size_t bytes_recvd) -> void +{ + if (!_running) return; + + if (!error) + { + if (_tunnel_source && _tunnel_sender_endpoint.address() != *_tunnel_source) { + // The "extra address checking" this issue asks for on top of whatever packet_modifier + // does -- see tunnel_source's comment in the header: this is a generic, + // encapsulation-agnostic admission check the library can reasonably own itself, unlike + // parsing any particular header format. + spdlog::warn("Receiver: discarding tunnel datagram from unexpected source {} (expected {})", + _tunnel_sender_endpoint.address().to_string(), _tunnel_source->to_string()); + } else { + _tunnel_data.resize(bytes_recvd); + size_t payload_offset = _packet_modifier(_tunnel_data); + if (payload_offset < _tunnel_data.size()) { + process_alc_datagram(reinterpret_cast(_tunnel_data.data() + payload_offset), _tunnel_data.size() - payload_offset); + } else { + spdlog::trace("Receiver: packet_modifier reported no usable payload in tunnel datagram, discarding"); + } + } + + arm_tunnel_receive(); + } + else + { + // BUG FIX (found live, 2026-08-11): this used to just log and return, never re-arming -- + // a single transient socket error (e.g. an ICMP port-unreachable surfacing as a UDP + // socket error on a subsequent read, confirmed live with the raw-capture-relay's + // loopback sendto() path) permanently killed tunnel reception for the rest of the + // process's life, with no further log output and no way to recover short of restarting + // rt-mbs-client. operation_aborted is the one error that means "don't re-arm" (this + // Receiver, or its socket, is being torn down -- re-arming here would race the + // destructor); every other error is presumed transient and worth retrying. + if (error != boost::asio::error::operation_aborted) { + spdlog::error("tunnel receive_from error: {} -- re-arming", error.message()); + arm_tunnel_receive(); + } else { + spdlog::error("tunnel receive_from error: {}", error.message()); + } + } +} + auto LibFlute::Receiver::enable_ipsec(uint32_t spi, const std::string& key, const std::string& auth_key) -> void { LibFlute::IpSec::enable_esp(spi, _mcast_address, _mcast_port, LibFlute::IpSec::Direction::In, @@ -219,6 +308,33 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er if (!error) { + process_alc_datagram(_data, bytes_recvd); + arm_receive(); + } + else + { + // BUG FIX: see the matching NOTE in handle_tunnel_receive_from() -- same "never re-arms + // on error" bug, same fix (re-arm on anything except operation_aborted). + if (error != boost::asio::error::operation_aborted) { + spdlog::error("receive_from error: {} -- re-arming", error.message()); + arm_receive(); + } else { + spdlog::error("receive_from error: {}", error.message()); + } + } +} + +// The actual ALC/FLUTE processing, shared by the normal multicast-socket path +// (handle_receive_from(), unmodified) and the tunnel path (handle_tunnel_receive_from(), +// after packet_modifier_t has located the payload inside whatever encapsulation wrapped it -- +// see packet_modifier_t's comment in the header for why these are two independent, +// simultaneously-armed receive loops feeding into this one function rather than two entirely +// separate copies of it). +auto LibFlute::Receiver::process_alc_datagram(char* data, size_t bytes_recvd) -> void +{ + /* Discards below return rather than arming: both receive loops arm their own socket after + calling this, so arming here would leave two outstanding reads on the plain socket, and would + arm the plain socket from a datagram that arrived through the tunnel. */ spdlog::trace("Received {} bytes", bytes_recvd); /* The source is checked before the packet is parsed, so traffic that is not this session's cannot reach the parser at all, let alone influence how a parse failure is handled. @@ -235,12 +351,11 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er if (_expected_source && _sender_endpoint.address() != *_expected_source) { spdlog::warn("Discarding packet from {}, which is not this session's source {}", _sender_endpoint.address().to_string(), _expected_source->to_string()); - arm_receive(); return; } try { - auto alc = LibFlute::AlcPacket(_data, bytes_recvd); + auto alc = LibFlute::AlcPacket(data, bytes_recvd); if (alc.tsi() == _tsi) { @@ -269,7 +384,6 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er its session, and RFC 3926 clause 3.1 is what makes such a peer send one. */ const size_t payload_len = bytes_recvd - alc.header_length(); if (payload_len == 0) { - arm_receive(); return; } @@ -281,7 +395,6 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er if (payload_len < 4) { spdlog::warn("Discarding a {}-byte payload, too short to hold a FEC Payload ID", payload_len); - arm_receive(); return; } @@ -328,11 +441,24 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er // the FDT once it arrives (see the merge below) or left blank if it never does. FileDeliveryTable::FileEntry fe{static_cast(alc.toi()), "", static_cast(alc.fec_oti().transfer_length), "", "", 0, alc.fec_oti()}; _files[alc.toi()] = std::make_shared(fe); + + /* The FDT may already have described this object and been unable to say how long it is, + in which case its entry is waiting here rather than lost: adopt it now, so the object + is written to its Content-Location and decoded per its Content-Encoding instead of + landing anonymous and still compressed. */ + if (_fdt) { + for (const auto& entry : _fdt->file_entries()) { + if (entry.toi == alc.toi()) { + _files[alc.toi()]->adopt_fdt_metadata(entry); + break; + } + } + } } if (_files.find(alc.toi()) != _files.end() && !_files[alc.toi()]->complete()) { auto encoding_symbols = LibFlute::EncodingSymbol::from_payload( - _data + alc.header_length(), + data + alc.header_length(), payload_len, _files[alc.toi()]->fec_oti(), alc.content_encoding()); @@ -404,6 +530,18 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er existing_file = _files.end(); } if (existing_file == _files.end()) { + if (file_entry.fec_oti.transfer_length == 0) { + /* The FDT does not say how long this object is on the wire, which happens for a + content-encoded object under the MBMS Download Profile: TS 26.346 V18.2.0 + clause L.4.4 forbids the sender from carrying Transfer-Length, and RFC 3926 + clause 3.4.2 only lets Content-Length stand in when no encoding was applied. + Starting reception now would mean partitioning the object to a length that is + simply unknown. Wait instead: the object's own EXT_FTI carries the length, and + the branch above picks this entry's metadata up again once it arrives. */ + spdlog::debug("Deferring reception for TOI {}: transfer length not in the FDT, " + "awaiting the object's EXT_FTI", file_entry.toi); + continue; + } spdlog::debug("Starting reception for file with TOI {}: {} ({})", file_entry.toi, file_entry.content_location, file_entry.content_type); _files.emplace(file_entry.toi, std::make_shared(file_entry)); @@ -427,13 +565,6 @@ auto LibFlute::Receiver::handle_receive_from(const boost::system::error_code& er // std::terminate(). spdlog::warn("Failed to decode ALC/FLUTE packet: {}", ex); } - - arm_receive(); - } - else - { - spdlog::error("receive_from error: {}", error.message()); - } } auto LibFlute::Receiver::file_list() -> std::vector> diff --git a/src/Transmitter.cpp b/src/Transmitter.cpp index 4ee8eed..0d14817 100644 --- a/src/Transmitter.cpp +++ b/src/Transmitter.cpp @@ -737,6 +737,25 @@ auto Transmitter::send( auto Transmitter::send(const std::shared_ptr &file_description) -> uint16_t { + /* The MBMS Download Profile permits content encoding but provides no carrier for the resulting + transfer length, so this sender does not use it there. TS 26.346 V18.2.0 clause L.4.2 makes it + a sender's choice, which is what makes declining it conformant: "The following FDT attribute, + defined at both the FDT-Instance and File levels, may be carried in the FDT sent by the FLUTE + sender". Refusing rather than silently dropping the encoding, because a caller that asked for + compression and got an uncompressed object with no warning has been misled. + + Clause L.4.4 forbids Transfer-Length in the FDT and clause 7.2.8 forbids EXT_FTI on a content + packet, and RFC 3926 clause 3.4.2 lets Content-Length stand in only when no encoding was + applied, so an encoded object under this profile cannot state its length by any route. Raised + as 5G-MAG/Standards#212. Receiving a content-encoded object is unaffected: L.4.2 requires a + receiver to support gzip and this library does. */ + if (is_3gpp(_profile) && !file_description->file_entry().content_encoding.empty()) { + throw std::runtime_error( + "Content encoding is not used by this sender under the 3GPP profiles, which provide " + "no way to carry the resulting transfer length. See 5G-MAG/Standards#212, and the " + "citations at this check. Use Profile::Unprofiled, or send the object uncompressed."); + } + if (file_description->has_tsi() && file_description->tsi() != _tsi) { // Reset TOI if the file_description is being used on a new TSI file_description->toi(0); @@ -850,8 +869,25 @@ auto Transmitter::send_next_packet() -> void for(const auto& symbol : symbols) { spdlog::debug("sending TOI {} SBN {} ID {}", file->meta().toi, symbol.source_block_number(), symbol.id() ); } + /* EXT_FTI is never attached to a content packet under a 3GPP profile. + + TS 26.346 V18.2.0 clause 7.2.8: "-FLUTE packets carrying symbols of files (not FDT + Instances) shall not include an EXT_FTI." + + This closes the second of the two carriers a content-encoded object's transfer length could + use, the first being the FDT's Transfer-Length attribute, which clause L.4.4 forbids. The + profile nonetheless permits gzip, so it allows a case it provides no way to signal. Raised + as 5G-MAG/Standards#212. Until that is answered the sender does not content encode under + these profiles at all, see Transmitter::send(), so the case does not arise; outside them + both the encoding and this extension remain available. */ + const bool fti_on_content_packet = file->meta().toi != 0 && + !is_3gpp(_profile) && + !file->meta().content_encoding.empty(); + auto packet = std::make_shared(_tsi, file->meta().toi, file->meta().fec_oti, symbols, _max_payload, file->fdt_instance_id(), - _session_closing, _closing_objects.count(file->meta().toi) > 0); + _session_closing, _closing_objects.count(file->meta().toi) > 0, + fti_on_content_packet); + bytes_queued += packet->size(); /* A tunnel is an additional path, not a replacement for the announced one. Sending only the diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index eb70162..6e6ce17 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -48,3 +48,4 @@ add_flute_test_executable(flute_unit_tests test_transmitter.cpp "unit:") add_flute_test_executable(flute_e2e_tests test_end_to_end.cpp "e2e:") add_flute_test_executable(flute_protocol_tests test_protocol_fixes.cpp "protocol:") add_flute_test_executable(flute_fdt_growth_tests test_fdt_growth.cpp "fdt_growth:") +add_flute_test_executable(flute_compression_tests test_compression.cpp "compression:") diff --git a/tests/test_compression.cpp b/tests/test_compression.cpp new file mode 100644 index 0000000..3fa43fe --- /dev/null +++ b/tests/test_compression.cpp @@ -0,0 +1,119 @@ +// libflute - FLUTE/ALC library +// +// Tests for source block partitioning's divisors. +// +// File::encode(), the Content-Encoding path, is deliberately NOT covered here. It runs inside the +// File constructor and the only way to reach it with a usable FEC OTI is through +// Transmitter::FileDescription, whose merge_fec_oti() is protected, so a unit test cannot +// configure one without standing up a Transmitter with its sockets and io_context. That gap is +// recorded rather than worked around; see the commit that fixed the compress loop. + +#include + +#include +#include +#include + +#include + +#include +#include + +#include "File.h" +#include "Transmitter.h" + +using namespace LibFlute; + +namespace { + +FecOti oti_with(uint32_t encoding_symbol_length, uint32_t max_source_block_length) { + FecOti oti{}; + oti.encoding_id = FecScheme::CompactNoCode; + oti.transfer_length = 4096; + oti.encoding_symbol_length = encoding_symbol_length; + oti.max_source_block_length = max_source_block_length; + return oti; +} + +std::shared_ptr build(const FecOti &oti, std::vector &data) { + return std::make_shared(/*toi*/1, oti, "http://example.invalid/o", + "application/octet-stream", /*expires*/0, + data.data(), data.size(), /*copy_data*/true); +} + +} // namespace + +/* Both values are used as denominators in RFC 5052 clause 9.1 partitioning. A default-constructed + FecOti leaves them 0, which made the first division produce inf, the block count inf, and block + creation effectively unbounded, so the constructor hung instead of reporting anything. The + public constructors accept a FecOti without inspecting it, so this is reachable by a caller. */ + +TEST(PartitioningDivisorTest, UsableFecOtiIsAccepted) { + std::vector data(4096, 'x'); + EXPECT_NO_THROW(build(oti_with(1400, 64), data)); +} + +TEST(PartitioningDivisorTest, ZeroEncodingSymbolLengthIsRefusedNotHung) { + std::vector data(4096, 'x'); + EXPECT_THROW(build(oti_with(0, 64), data), std::runtime_error); +} + +TEST(PartitioningDivisorTest, ZeroMaxSourceBlockLengthIsRefusedNotHung) { + std::vector data(4096, 'x'); + EXPECT_THROW(build(oti_with(1400, 0), data), std::runtime_error); +} + +TEST(PartitioningDivisorTest, DefaultConstructedFecOtiIsRefused) { + // The exact shape that hung: nothing configured at all. + std::vector data(4096, 'x'); + FecOti bare{}; + bare.encoding_id = FecScheme::CompactNoCode; + bare.transfer_length = 4096; + EXPECT_THROW(build(bare, data), std::runtime_error); +} + + +/* The MBMS Download Profile permits content encoding and provides no carrier for the resulting + transfer length, so this sender declines it there. TS 26.346 V18.2.0 clause L.4.4 forbids + Transfer-Length in the FDT, clause 7.2.8 forbids EXT_FTI on a content packet, and RFC 3926 clause + 3.4.2 lets Content-Length stand in only where no encoding was applied. Raised as + 5G-MAG/Standards#212. Declining to encode is conformant, the attribute being optional for a + sender; the verbatim clauses are quoted at the check itself in Transmitter.cpp. */ +namespace { + +std::shared_ptr gzipped_file() { + const std::vector payload(4096, 'x'); + auto fd = std::make_shared("test/compressible.bin", payload); + fd->set_compression(Transmitter::FileDescription::COMPRESSION_GZIP); + return fd; +} + +} // namespace + +TEST(ProfileContentEncodingTest, RefusedUnderThe3gppProfiles) { + boost::asio::io_context io; + Transmitter tx("239.1.2.30", 5000, /*tsi*/ 1, /*mtu*/ 1400, /*rate_limit*/ 0, io, + /*tunnel_endpoint*/ std::nullopt, FileDeliveryTable::FDT_NS_NONE, + /*active*/ false, /*source_address*/ std::nullopt, Profile::Ts26517); + EXPECT_THROW(tx.send(gzipped_file()), std::runtime_error) + << "a gzip-encoded object was accepted under a profile that cannot carry its transfer length"; +} + +TEST(ProfileContentEncodingTest, AllowedOutsideTheProfile) { + boost::asio::io_context io; + Transmitter tx("239.1.2.31", 5000, /*tsi*/ 1, /*mtu*/ 1400, /*rate_limit*/ 0, io, + /*tunnel_endpoint*/ std::nullopt, FileDeliveryTable::FDT_NS_NONE, + /*active*/ false, /*source_address*/ std::nullopt, Profile::Unprofiled); + EXPECT_NO_THROW(tx.send(gzipped_file())) + << "plain RFC 3926 permits Content-Encoding, and Transfer-Length with it"; +} + +TEST(ProfileContentEncodingTest, AnUnencodedObjectIsUnaffected) { + boost::asio::io_context io; + Transmitter tx("239.1.2.32", 5000, /*tsi*/ 1, /*mtu*/ 1400, /*rate_limit*/ 0, io, + /*tunnel_endpoint*/ std::nullopt, FileDeliveryTable::FDT_NS_NONE, + /*active*/ false, /*source_address*/ std::nullopt, Profile::Ts26517); + const std::vector payload(4096, 'y'); + auto fd = std::make_shared("test/plain.bin", payload); + EXPECT_NO_THROW(tx.send(fd)); +} diff --git a/tests/test_protocol_fixes.cpp b/tests/test_protocol_fixes.cpp index 7bbef13..95c47de 100644 --- a/tests/test_protocol_fixes.cpp +++ b/tests/test_protocol_fixes.cpp @@ -880,3 +880,48 @@ TEST(DataLessClosePacket, DoesNotDisturbALiveReceiver) { io.stop(); io_thread.join(); } + +// Whether Content-Length may stand in for a missing Transfer-Length depends on +// whether the object was content encoded, and on nothing else. +// RFC 3926 clause 3.4.2: "If the file is not content encoded before transport +// (and thus the "Content-Encoding" attribute is not used) then the transfer +// length is the length of the original file, and in this case the +// "Content-Length" is also the transfer length." +namespace { +std::string fdt_with(const std::string& file_attrs) { + return std::string("" + "" + ""; +} + +uint64_t parsed_transfer_length(const std::string& file_attrs) { + auto xml = fdt_with(file_attrs); + std::vector buf(xml.begin(), xml.end()); + LibFlute::FileDeliveryTable fdt(1, buf.data(), buf.size()); + for (const auto& e : fdt.file_entries()) { + if (e.toi == 1) return e.fec_oti.transfer_length; + } + throw std::runtime_error("no entry parsed"); +} +} // namespace + +TEST(EncodedObjectTransferLengthTest, ContentLengthStandsInOnlyWithoutAnEncoding) { + // No encoding: the clause authorises the substitution. + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\""), 5000u); +} + +TEST(EncodedObjectTransferLengthTest, AnEncodedObjectDoesNotBorrowContentLength) { + // With an encoding the two lengths differ, so borrowing Content-Length would + // hand the decoder a length wrong by however much the encoding changed. The + // length is left unknown for the object's own EXT_FTI to supply. + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\" Content-Encoding=\"gzip\""), 0u) + << "an encoded object's transfer length is not its Content-Length"; +} + +TEST(EncodedObjectTransferLengthTest, AnExplicitTransferLengthAlwaysWins) { + EXPECT_EQ(parsed_transfer_length("Content-Length=\"5000\" Transfer-Length=\"4096\""), 4096u); + EXPECT_EQ(parsed_transfer_length( + "Content-Length=\"5000\" Transfer-Length=\"4096\" Content-Encoding=\"gzip\""), 4096u); +}