Limit source names to 63 bytes so they stay null-terminated

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());
 

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<SacnSource> {
-        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(),
             ));

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.

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();
 

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(),
+                },
+            }),
+        },
+    }
+}