Repository: informalsystems/tendermint-rs (language: rust)

Issue:
p2p: failure to read complete message
### What went wrong?

There are ongoing reports of failure to read a complete message in the `tendermint-p2p` crate which seem to be manifesting as an incomplete Protobuf message in TMKMS:

https://github.com/iqlusioninc/tmkms/issues/729

### Steps to reproduce

Unfortunately this seems very chain-specific and I have not been given proper repro steps myself in the aforementioned issue.

### Definition of "done"

It's unclear what actually needs to change here.

A somewhat related issue is the API that `tendermint-p2p` exposes: it's a message-oriented protocol, but the interface uses the `std::io::{Read, Write}` traits, which are stream-oriented. It's possible TMKMS is misusing the API, but it's the wrong abstraction to begin with.


Discussion / hints:
This might be related to #1356, although curiously people claim things work with Unix domain sockets and the errors only occur over TCP.

I did confirm `read_exact` is used: https://github.com/informalsystems/tendermint-rs/blob/64e9b33/p2p/src/secret_connection.rs#L622

Fix patch:
diff --git a/.changelog/unreleased/bug-fixes/1393-p2p-data-corruption.md b/.changelog/unreleased/bug-fixes/1393-p2p-data-corruption.md
new file mode 100644
index 000000000..fcea9ebc7
--- /dev/null
+++ b/.changelog/unreleased/bug-fixes/1393-p2p-data-corruption.md
@@ -0,0 +1,2 @@
+- `[tendermint-p2p]` Fix data corruption on sending long messages via `SecretConnection`
+  ([\#1393](https://github.com/informalsystems/tendermint-rs/pull/1393))
diff --git a/p2p/src/secret_connection.rs b/p2p/src/secret_connection.rs
index 807318087..1b30c73a1 100644
--- a/p2p/src/secret_connection.rs
+++ b/p2p/src/secret_connection.rs
@@ -538,16 +538,7 @@ fn encrypt_and_write<IoHandler: Write>(
     data: &[u8],
 ) -> io::Result<usize> {
     let mut n = 0_usize;
-    let mut data_copy = data;
-    while !data_copy.is_empty() {
-        let chunk: &[u8];
-        if DATA_MAX_SIZE < data.len() {
-            chunk = &data[..DATA_MAX_SIZE];
-            data_copy = &data_copy[DATA_MAX_SIZE..];
-        } else {
-            chunk = data_copy;
-            data_copy = &[0_u8; 0];
-        }
+    for chunk in data.chunks(DATA_MAX_SIZE) {
         let sealed_frame = &mut [0_u8; TAG_SIZE + TOTAL_FRAME_SIZE];
         encrypt(chunk, &send_state.cipher, &send_state.nonce, sealed_frame)
             .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;


Test patch:
diff --git a/test/src/test/unit/p2p/secret_connection.rs b/test/src/test/unit/p2p/secret_connection.rs
index 410e78ace..f7d88c992 100644
--- a/test/src/test/unit/p2p/secret_connection.rs
+++ b/test/src/test/unit/p2p/secret_connection.rs
@@ -62,6 +62,35 @@ fn test_read_write_single_message() {
     receiver.join().expect("receiver thread has panicked");
 }
 
+#[test]
+fn test_read_write_long_message() {
+    let mut message: [u8; 1025] = [0x5a; 1025];
+    message[1024] = 0xa5;
+
+    let (pipe1, pipe2) = pipe::async_bipipe_buffered();
+
+    let sender = thread::spawn(move || {
+        let mut conn1 = new_peer_conn(pipe2).expect("handshake to succeed");
+
+        conn1
+            .write_all(&message)
+            .expect("expected to write message");
+    });
+
+    let receiver = thread::spawn(move || {
+        let mut conn2 = new_peer_conn(pipe1).expect("handshake to succeed");
+
+        let mut buf = [0; 1025];
+        conn2
+            .read_exact(&mut buf)
+            .expect("expected to read message");
+        assert_eq!(&message, &buf);
+    });
+
+    sender.join().expect("sender thread has panicked");
+    receiver.join().expect("receiver thread has panicked");
+}
+
 #[test]
 fn test_evil_peer_shares_invalid_eph_key() {
     let csprng = OsRng {};