From f2dab39e20ea140c2093d8b47b170048be5a3d03 Mon Sep 17 00:00:00 2001
From: Uwe Klotz <uwe.klotz@slowtec.de>
Date: Tue, 15 Oct 2024 11:47:26 +0200
Subject: [PATCH] Verify exact frame length on decoding

---
 CHANGELOG.md     |  2 +-
 src/codec/mod.rs | 52 ++++++++++++++++++++----------------------------
 2 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 81a4194..d555632 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,7 +6,7 @@
 ## v0.15.1 (Unreleased)
 
 - Request encoding: Reduced allocations
-- Response decoding: More strict validation
+- Request/response decoding: More strict validation
 
 ## v0.15.0 (2024-10-10)
 
diff --git a/src/codec/mod.rs b/src/codec/mod.rs
index 3640a1c..b5ae00a 100644
--- a/src/codec/mod.rs
+++ b/src/codec/mod.rs
@@ -183,7 +183,7 @@ impl TryFrom<Bytes> for Request<'static> {
                 let quantity = read_u16_be(rdr)?;
                 let byte_count = rdr.read_u8()?;
                 if bytes.len() < 6 + usize::from(byte_count) {
-                    return Err(Error::new(ErrorKind::InvalidData, "Invalid byte count"));
+                    return Err(Error::new(ErrorKind::InvalidData, "too short"));
                 }
                 let x = &bytes[6..];
                 WriteMultipleCoils(address, decode_packed_coils(x, quantity).into())
@@ -197,7 +197,7 @@ impl TryFrom<Bytes> for Request<'static> {
                 let quantity = read_u16_be(rdr)?;
                 let byte_count = rdr.read_u8()?;
                 if bytes.len() < 6 + usize::from(byte_count) {
-                    return Err(Error::new(ErrorKind::InvalidData, "Invalid byte count"));
+                    return Err(Error::new(ErrorKind::InvalidData, "too short"));
                 }
                 let mut data = Vec::with_capacity(quantity.into());
                 for _ in 0..quantity {
@@ -219,7 +219,7 @@ impl TryFrom<Bytes> for Request<'static> {
                 let write_quantity = read_u16_be(rdr)?;
                 let write_count = rdr.read_u8()?;
                 if bytes.len() < 10 + usize::from(write_count) {
-                    return Err(Error::new(ErrorKind::InvalidData, "Invalid byte count"));
+                    return Err(Error::new(ErrorKind::InvalidData, "too short"));
                 }
                 let mut data = Vec::with_capacity(write_quantity.into());
                 for _ in 0..write_quantity {
@@ -227,14 +227,23 @@ impl TryFrom<Bytes> for Request<'static> {
                 }
                 ReadWriteMultipleRegisters(read_address, read_quantity, write_address, data.into())
             }
-            fn_code if fn_code < 0x80 => Custom(fn_code, bytes[1..].to_vec().into()),
+            fn_code if fn_code < 0x80 => {
+                return Ok(Custom(fn_code, bytes[1..].to_vec().into()));
+            }
             fn_code => {
                 return Err(Error::new(
                     ErrorKind::InvalidData,
-                    format!("Invalid function code: 0x{fn_code:0>2X}"),
+                    format!("invalid function code: 0x{fn_code:02X}"),
                 ));
             }
         };
+        // Verify that all data has been consumed and decoded.
+        if rdr.has_remaining() {
+            return Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "undecoded request data",
+            ));
+        }
         Ok(req)
     }
 }
@@ -300,12 +309,6 @@ impl TryFrom<Bytes> for Response {
                 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 => {
@@ -321,12 +324,6 @@ impl TryFrom<Bytes> for Response {
                 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)?),
@@ -353,12 +350,6 @@ impl TryFrom<Bytes> for Response {
                 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 => {
@@ -380,19 +371,20 @@ impl TryFrom<Bytes> for Response {
                 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)
             }
             _ => {
                 let mut bytes = bytes;
-                Custom(fn_code, bytes.split_off(1))
+                return Ok(Custom(fn_code, bytes.split_off(1)));
             }
         };
+        // Verify that all data has been consumed and decoded.
+        if rdr.has_remaining() {
+            return Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "undecoded response data",
+            ));
+        }
         Ok(rsp)
     }
 }