diff --git a/src/packet.rs b/src/packet.rs index 74789d5..c79d09a 100644 --- a/src/packet.rs +++ b/src/packet.rs @@ -248,6 +248,11 @@ const E131_CID_END_INDEX: usize = /// The length of the Source Name field in bytes in an ANSI E1.31-2018 packet as per ANSI E1.31-2018 Section 4, Table 4-1, 4-2, 4-3. pub const E131_SOURCE_NAME_FIELD_LENGTH: usize = 64; +/// The maximum length of a source name in bytes (UTF-8), as per ANSI E1.31-2016 Section 6.2.2. +/// +/// The source name field is 64 bytes and must be null-terminated, which leaves 63 bytes for the name itself. +pub const E131_SOURCE_NAME_MAX_LENGTH: usize = E131_SOURCE_NAME_FIELD_LENGTH - 1; + /// The length of the Synchronisation Address field in bytes in an ANSI E1.31-2018 packet as per ANSI E1.31-2018 Section 4, Table 4-1, 4-2, 4-3. pub const E131_SYNC_ADDR_FIELD_LENGTH: usize = 2; @@ -832,7 +837,12 @@ macro_rules! impl_data_packet_framing_layer { // Vector NetworkEndian::write_u32(&mut buf[E131_PDU_LENGTH_FLAGS_LENGTH .. SOURCE_NAME_INDEX], VECTOR_E131_DATA_PACKET); - // Source Name, padded with 0's up to the required 64 byte length. + // Source Name, padded with 0's up to the required 64 byte length, always leaving room for the null terminator. + if self.source_name.len() > E131_SOURCE_NAME_MAX_LENGTH { + return Err(SacnError::SacnParsePackError(ParsePacketError::PackInvalidData( + format!("Source name must be at most {} bytes", E131_SOURCE_NAME_MAX_LENGTH) + ))); + } zeros(&mut buf[SOURCE_NAME_INDEX .. PRIORITY_INDEX], E131_SOURCE_NAME_FIELD_LENGTH); buf[SOURCE_NAME_INDEX .. SOURCE_NAME_INDEX + self.source_name.len()].copy_from_slice(self.source_name.as_bytes()); @@ -1263,7 +1273,12 @@ macro_rules! impl_universe_discovery_packet_framing_layer { // Vector NetworkEndian::write_u32(&mut buf[E131_DISCOVERY_FRAMING_LAYER_VECTOR_FIELD_INDEX .. E131_DISCOVERY_FRAMING_LAYER_SOURCE_NAME_FIELD_INDEX], VECTOR_E131_EXTENDED_DISCOVERY); - // Source Name + // Source Name, always leaving room for the null terminator. + if self.source_name.len() > E131_SOURCE_NAME_MAX_LENGTH { + return Err(SacnError::SacnParsePackError(ParsePacketError::PackInvalidData( + format!("Source name must be at most {} bytes", E131_SOURCE_NAME_MAX_LENGTH) + ))); + } zeros(&mut buf[E131_DISCOVERY_FRAMING_LAYER_SOURCE_NAME_FIELD_INDEX .. E131_DISCOVERY_FRAMING_LAYER_RESERVE_FIELD_INDEX], E131_SOURCE_NAME_FIELD_LENGTH); buf[E131_DISCOVERY_FRAMING_LAYER_SOURCE_NAME_FIELD_INDEX .. E131_DISCOVERY_FRAMING_LAYER_SOURCE_NAME_FIELD_INDEX + self.source_name.len()].copy_from_slice(self.source_name.as_bytes()); diff --git a/src/source.rs b/src/source.rs index f701751..ccb346d 100644 --- a/src/source.rs +++ b/src/source.rs @@ -192,9 +192,9 @@ impl SacnSource { /// /// `UnsupportedIpVersion`: Returned if the `SocketAddr` is not IPv4 or IPv6. /// - /// `MalformedSourceName`: Returned if the given source name is longer than the maximum allowed size of `E131_SOURCE_NAME_FIELD_LENGTH`. + /// `MalformedSourceName`: Returned if the given source name is longer than the maximum allowed size of `E131_SOURCE_NAME_MAX_LENGTH` (63 bytes, leaving room for the null terminator). pub fn with_cid_ip(name: &str, cid: Uuid, ip: SocketAddr) -> Result { - if name.len() > E131_SOURCE_NAME_FIELD_LENGTH { + if name.len() > E131_SOURCE_NAME_MAX_LENGTH { return Err(SacnError::MalformedSourceName( "Source name provided is longer than maximum allowed".to_string(), )); @@ -404,7 +404,7 @@ impl SacnSource { /// `SourceCorrupt`: Returned if the Mutex used to control access to the internal sender is poisoned by a thread encountering /// a panic while accessing causing the source to be left in a potentially inconsistent state. /// - /// `MalformedSourceName`: Returned to indicate that the given source name is longer than the maximum allowed as per `E131_SOURCE_NAME_FIELD_LENGTH`. + /// `MalformedSourceName`: Returned to indicate that the given source name is longer than the maximum allowed as per `E131_SOURCE_NAME_MAX_LENGTH` (63 bytes, leaving room for the null terminator). pub fn set_name(&mut self, name: &str) -> Result<()> { unlock_internal_mut(&mut self.internal)?.set_name(name) } @@ -1143,9 +1143,9 @@ impl SacnSourceInternal { /// name: The new name for the source, it is left to the user to ensure this is unique within the sACN network. /// /// # Errors - /// `MalformedSourceName`: Returned to indicate that the given source name is longer than the maximum allowed as per `E131_SOURCE_NAME_FIELD_LENGTH`. + /// `MalformedSourceName`: Returned to indicate that the given source name is longer than the maximum allowed as per `E131_SOURCE_NAME_MAX_LENGTH` (63 bytes, leaving room for the null terminator). fn set_name(&mut self, name: &str) -> Result<()> { - if name.len() > E131_SOURCE_NAME_FIELD_LENGTH { + if name.len() > E131_SOURCE_NAME_MAX_LENGTH { return Err(SacnError::MalformedSourceName( "Source name provided is longer than maximum allowed".to_string(), )); diff --git a/tests/ipv4_tests.rs b/tests/ipv4_tests.rs index 7cf5c28..bf17e14 100644 --- a/tests/ipv4_tests.rs +++ b/tests/ipv4_tests.rs @@ -3311,7 +3311,7 @@ fn test_data_packet_transmit_format() { let packet = generate_data_packet_raw(CID, universe, source_name.clone(), PRIORITY, sequence, OPTIONS, dmx_data.clone()); let ip: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), ACN_SDT_MULTICAST_PORT + 1); - let mut source = SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ip).unwrap(); + let mut source = SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ip).unwrap(); source.set_preview_mode(false).unwrap(); source.set_multicast_loop_v4(true).unwrap(); @@ -3509,7 +3509,7 @@ fn test_discovery_packet_transmit_format() { let ip: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), ACN_SDT_MULTICAST_PORT + 1); // Creates the source. - let mut source = SacnSource::with_cid_ip(&str::from_utf8(&SOURCE_NAME).unwrap(), Uuid::from_bytes(CID), ip).unwrap(); + let mut source = SacnSource::with_cid_ip(str::from_utf8(&SOURCE_NAME).unwrap().trim_end_matches('\0'), Uuid::from_bytes(CID), ip).unwrap(); source.set_multicast_loop_v4(true).unwrap(); @@ -3668,7 +3668,7 @@ fn test_track_data_packet_seq_numbers() { // Create a source. let ip: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), ACN_SDT_MULTICAST_PORT + 1); - let mut source = SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ip).unwrap(); + let mut source = SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ip).unwrap(); source.set_multicast_loop_v4(true).unwrap(); source.register_universes(&UNIVERSES).unwrap(); @@ -3735,7 +3735,7 @@ fn test_track_sync_packet_seq_numbers() { // Create a source. let ip: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), ACN_SDT_MULTICAST_PORT + 1); - let mut source = SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ip).unwrap(); + let mut source = SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ip).unwrap(); source.set_multicast_loop_v4(true).unwrap(); // Register the synchronisation addresses. @@ -3810,7 +3810,7 @@ fn test_sync_packet_multicast_address() { // Create a source. let ip: SocketAddr = SocketAddr::new(IpAddr::V4(TEST_NETWORK_INTERFACE_IPV4[0].parse().unwrap()), ACN_SDT_MULTICAST_PORT + 1); - let mut source = SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ip).unwrap(); + let mut source = SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ip).unwrap(); source.set_multicast_loop_v4(true).unwrap(); // Register the synchronisation addresses. diff --git a/tests/ipv6_tests.rs b/tests/ipv6_tests.rs index 55acd34..e3f389a 100644 --- a/tests/ipv6_tests.rs +++ b/tests/ipv6_tests.rs @@ -1624,7 +1624,7 @@ mod sacn_ipv6_multicast_test { ACN_SDT_MULTICAST_PORT + 1, ); let mut ipv4_source = - SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ipv4).unwrap(); + SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ipv4).unwrap(); ipv4_source.set_preview_mode(false).unwrap(); ipv4_source.set_multicast_loop_v4(true).unwrap(); ipv4_source.register_universes(&[universe as u16]).unwrap(); @@ -1665,7 +1665,7 @@ mod sacn_ipv6_multicast_test { ACN_SDT_MULTICAST_PORT + 1, ); let mut ipv6_source = - SacnSource::with_cid_ip(&source_name.clone(), Uuid::from_bytes(CID), ipv6).unwrap(); + SacnSource::with_cid_ip(source_name.trim_end_matches('\0'), Uuid::from_bytes(CID), ipv6).unwrap(); ipv6_source.set_preview_mode(false).unwrap(); ipv6_source.register_universes(&[universe]).unwrap(); diff --git a/tests/src_tests.rs b/tests/src_tests.rs index ee9605f..fe8f248 100644 --- a/tests/src_tests.rs +++ b/tests/src_tests.rs @@ -702,3 +702,100 @@ const TEST_DATA_MULTIPLE_UNIVERSE: [u8; 712] = [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 100, ]; + +/// A 64 byte source name fills the whole field and leaves no room for the null terminator required by +/// ANSI E1.31-2016 Section 6.2.2, so it must be rejected. +#[test] +#[serial] +fn test_new_ipv4_64_byte_source_name_rejected() { + let name = "a".repeat(E131_SOURCE_NAME_FIELD_LENGTH); + match SacnSource::new_v4(&name) { + Err(SacnError::MalformedSourceName(_)) => {} + Err(e) => panic!("Unexpected error returned: {e:?}"), + Ok(_) => panic!("SacnSource created with a 64 byte source name"), + } +} + +#[test] +#[serial] +fn test_new_ipv4_63_byte_source_name_accepted() { + let name = "a".repeat(E131_SOURCE_NAME_MAX_LENGTH); + let src = SacnSource::new_v4(&name).unwrap(); + assert_eq!(src.name().unwrap(), name); +} + +#[test] +#[serial] +fn test_set_name_64_byte_source_name_rejected() { + let mut src = SacnSource::new_v4("Source").unwrap(); + match src.set_name(&"a".repeat(E131_SOURCE_NAME_FIELD_LENGTH)) { + Err(SacnError::MalformedSourceName(_)) => {} + other => panic!("Expected MalformedSourceName, got {other:?}"), + } + assert_eq!(src.name().unwrap(), "Source"); +} + +/// A 63 byte source name packs into a packet that parses back to the same name. +#[test] +fn test_pack_parse_63_byte_source_name() { + let name = "a".repeat(E131_SOURCE_NAME_MAX_LENGTH); + let packet = data_packet_with_source_name(&name); + let buf = packet.pack_alloc().unwrap(); + assert_eq!(AcnRootLayerProtocol::parse(&buf).unwrap(), packet); +} + +/// Packing a source name that can't be null-terminated is an error rather than a non-compliant packet +/// (64 bytes) or a panic (65 or more bytes). +#[test] +fn test_pack_too_long_source_name_rejected() { + for len in [E131_SOURCE_NAME_FIELD_LENGTH, E131_SOURCE_NAME_FIELD_LENGTH + 1] { + let name = "a".repeat(len); + match data_packet_with_source_name(&name).pack_alloc() { + Err(SacnError::SacnParsePackError( + sacn::sacn_parse_pack_error::ParsePacketError::PackInvalidData(_), + )) => {} + other => panic!("Expected PackInvalidData for {len} byte name, got {other:?}"), + } + + let discovery = AcnRootLayerProtocol { + pdu: E131RootLayer { + cid: Uuid::new_v4(), + data: E131RootLayerData::UniverseDiscoveryPacket(UniverseDiscoveryPacketFramingLayer { + source_name: name.as_str().into(), + data: UniverseDiscoveryPacketUniverseDiscoveryLayer { + page: 0, + last_page: 0, + universes: vec![1u16].into(), + }, + }), + }, + }; + match discovery.pack_alloc() { + Err(SacnError::SacnParsePackError( + sacn::sacn_parse_pack_error::ParsePacketError::PackInvalidData(_), + )) => {} + other => panic!("Expected PackInvalidData for {len} byte name, got {other:?}"), + } + } +} + +fn data_packet_with_source_name(name: &str) -> AcnRootLayerProtocol<'_> { + AcnRootLayerProtocol { + pdu: E131RootLayer { + cid: Uuid::new_v4(), + data: E131RootLayerData::DataPacket(DataPacketFramingLayer { + source_name: name.into(), + priority: E131_DEFAULT_PRIORITY, + synchronization_address: 0, + sequence_number: 0, + preview_data: false, + stream_terminated: false, + force_synchronization: false, + universe: 1, + data: DataPacketDmpLayer { + property_values: vec![0u8; 513].into(), + }, + }), + }, + } +}