Repository: FactbirdHQ/atat (language: rust)

Issue:
`Ingress::read_from` causes busy-loop if write_buf is full
The current implementation of `Ingress::read_from` can cause the MCU to busy-loop: If for some reason the write_buf becomes full, the embedded_io_async::Read will return/error immediately, causing the MCU to busy-loop.

https://github.com/FactbirdHQ/atat/blob/2768e5ae760992641defc048972c2aa7e12043d2/atat/src/ingress.rs#L57-L74


As to why the write_buf became full in my case, I'm still investigating.


(CC @cacharle)


Discussion / hints:
Nice catch
Am I correct that ATAT should in theory be able to recover from garbage data at the beginning of the buffer?
If so, we could just print a warning and self.clear() if buf.len()==0. Or how can we fix this properly?
I would say so yeah.

I have been meaning to set up a fuzz test on it, to verify this behaviour.

Fix patch:
diff --git a/atat/src/ingress.rs b/atat/src/ingress.rs
index b99ca54..c2f8f87 100644
--- a/atat/src/ingress.rs
+++ b/atat/src/ingress.rs
@@ -58,6 +58,11 @@ pub trait AtatIngress {
         use embedded_io::Error;
         loop {
             let buf = self.write_buf();
+            if buf.is_empty() {
+                warn!("Ingress buffer full, clearing");
+                self.clear();
+                continue;
+            }
             match serial.read(buf).await {
                 Ok(received) => {
                     if received > 0 {
@@ -305,6 +310,7 @@ mod tests {
         self as atat, atat_derive::AtatUrc, digest::parser::take_until_including,
         response_slot::ResponseSlot, AtDigester, Response, UrcChannel,
     };
+    use embedded_io::ErrorType;
 
     use super::*;
 
@@ -449,4 +455,52 @@ mod tests {
         let response: &Response<100> = &response.borrow();
         assert_eq!(&Response::default(), response);
     }
+
+    #[tokio::test]
+    async fn read_from_can_recover_from_full_buffer() {
+        let res_slot = ResponseSlot::<30>::new();
+        let urc_channel = UrcChannel::<Urc, 100, 1>::new();
+        let mut buf = [0; 30];
+
+        let mut ingress: Ingress<_, Urc, 30, 100, 1> =
+            Ingress::new(AtDigester::<Urc>::new(), &mut buf, &res_slot, &urc_channel);
+
+        let mut sub = urc_channel.subscribe().unwrap();
+
+        struct Reader {
+            data: &'static [u8],
+            pos: usize,
+        }
+        impl ErrorType for Reader {
+            type Error = embedded_io::ErrorKind;
+        }
+        impl embedded_io_async::Read for Reader {
+            async fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
+                assert!(buf.len() > 0);
+                if self.pos >= self.data.len() {
+                    // Simulate waiting on more data.
+                    loop {
+                        tokio::task::yield_now().await
+                    }
+                }
+                let len = buf.len().min(self.data.len() - self.pos);
+                buf[..len].copy_from_slice(&self.data[self.pos..self.pos + len]);
+                self.pos += len;
+                Ok(len)
+            }
+        }
+
+        let mut r = Reader {
+            // 35 bytes of garbage, followed by a valid URC
+            data: b"123456789012345678901234567890aaaaa\r\nCONNECT OK\r\n",
+            pos: 0,
+        };
+
+        tokio::select! {
+            _ = ingress.read_from(&mut r) => {}
+            m = sub.next_message_pure() => {
+                assert_eq!(Urc::ConnectOk, m);
+            }
+        }
+    }
 }


Test patch: