Response decoding: More strict validation

slowtec · Oct 14, 2024 · a25a48e · a25a48e
1 parent 3af6122
commit a25a48e
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,11 @@
 
 # Changelog
 
+## v0.15.1 (Unreleased)
+
+- Request encoding: Reduced allocations
+- Response decoding: More strict validation
+
 ## v0.15.0 (2024-10-10)
 
 - Implement `Report Server ID` (function code 17).

diff --git a/src/codec/mod.rs b/src/codec/mod.rs
@@ -9,7 +9,7 @@ use std::{
 use byteorder::{BigEndian, ReadBytesExt as _};
 
 use crate::{
-    bytes::Bytes,
+    bytes::{Buf as _, Bytes},
     frame::{Coil, RequestPdu, ResponsePdu},
     ExceptionCode, ExceptionResponse, FunctionCode, Request, Response,
 };
@@ -251,68 +251,114 @@ impl TryFrom<Bytes> for RequestPdu<'static> {
 impl TryFrom<Bytes> for Response {
     type Error = Error;
 
+    #[allow(clippy::too_many_lines)] // TODO
     fn try_from(bytes: Bytes) -> Result<Self, Self::Error> {
         use crate::frame::Response::*;
         let rdr = &mut Cursor::new(&bytes);
         let fn_code = rdr.read_u8()?;
         let rsp = match fn_code {
             0x01 => {
                 let byte_count = rdr.read_u8()?;
-                let x = &bytes[2..];
+                let packed_coils = &bytes[2..];
+                if packed_coils.len() != byte_count.into() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 // Here we have not information about the exact requested quantity so we just
                 // unpack the whole byte.
                 let quantity = u16::from(byte_count) * 8;
-                ReadCoils(decode_packed_coils(x, quantity))
+                ReadCoils(decode_packed_coils(packed_coils, quantity))
             }
             0x02 => {
                 let byte_count = rdr.read_u8()?;
-                let x = &bytes[2..];
+                let packed_coils = &bytes[2..];
+                if packed_coils.len() != byte_count.into() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 // Here we have no information about the exact requested quantity so we just
                 // unpack the whole byte.
                 let quantity = u16::from(byte_count) * 8;
-                ReadDiscreteInputs(decode_packed_coils(x, quantity))
+                ReadDiscreteInputs(decode_packed_coils(packed_coils, quantity))
             }
             0x05 => WriteSingleCoil(read_u16_be(rdr)?, coil_to_bool(read_u16_be(rdr)?)?),
             0x0F => WriteMultipleCoils(read_u16_be(rdr)?, read_u16_be(rdr)?),
             0x04 => {
                 let byte_count = rdr.read_u8()?;
+                if byte_count % 2 != 0 {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 let quantity = byte_count / 2;
                 let mut data = Vec::with_capacity(quantity.into());
                 for _ in 0..quantity {
                     data.push(read_u16_be(rdr)?);
                 }
+                if rdr.has_remaining() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 ReadInputRegisters(data)
             }
             0x03 => {
                 let byte_count = rdr.read_u8()?;
+                if byte_count % 2 != 0 {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 let quantity = byte_count / 2;
                 let mut data = Vec::with_capacity(quantity.into());
                 for _ in 0..quantity {
                     data.push(read_u16_be(rdr)?);
                 }
+                if rdr.has_remaining() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 ReadHoldingRegisters(data)
             }
             0x06 => WriteSingleRegister(read_u16_be(rdr)?, read_u16_be(rdr)?),
 
             0x10 => WriteMultipleRegisters(read_u16_be(rdr)?, read_u16_be(rdr)?),
             0x11 => {
                 let byte_count = rdr.read_u8()?;
+                if byte_count < 2 {
+                    return Err(io::Error::new(io::ErrorKind::InvalidData, "too short"));
+                }
+                let data_len = (byte_count - 2).into();
                 let server_id = rdr.read_u8()?;
                 let run_indication_status = match rdr.read_u8()? {
                     0x00 => false,
                     0xFF => true,
                     status => {
                         return Err(Error::new(
                             ErrorKind::InvalidData,
-                            format!("Invalid run indication status value: {status}"),
+                            format!("invalid run indication status: 0x{status:02X}"),
                         ));
                     }
                 };
-                let data_len = byte_count.saturating_sub(2).into();
                 let mut data = Vec::with_capacity(data_len);
                 for _ in 0..data_len {
                     data.push(rdr.read_u8()?);
                 }
+                if rdr.has_remaining() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 ReportServerId(server_id, run_indication_status, data)
             }
             0x16 => {
@@ -323,11 +369,23 @@ impl TryFrom<Bytes> for Response {
             }
             0x17 => {
                 let byte_count = rdr.read_u8()?;
+                if byte_count % 2 != 0 {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 let quantity = byte_count / 2;
                 let mut data = Vec::with_capacity(quantity.into());
                 for _ in 0..quantity {
                     data.push(read_u16_be(rdr)?);
                 }
+                if rdr.has_remaining() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        "invalid quantity",
+                    ));
+                }
                 ReadWriteMultipleRegisters(data)
             }
             _ => {