feat(state format): correct {de-,}serialization of Friends

Spec was slightly wrong, new corrections make things finally work.

Author: zetok

Cargo.lock

@@ -7,6 +7,7 @@ authors = ["Zetok Zalbavar <[email protected]>"]
 [dependencies]
 clippy = { version = "*", optional = true }
 log = "0.3"
+num-traits = "0.1"
 sodiumoxide = "0.0.12"
 mio = { git = "https://github.com/carllerche/mio", rev = "bf5de4d51ee00f140c4fcbd73b4d53717a3ac9e3" }
 

Cargo.toml

@@ -27,7 +27,7 @@ Current [API docs](https://zetok.github.io/tox) are a subject to changes.
 | libsodium | >=1.0.0 |
 
 ## Building
-Fairly simple. You'll need [Rust] >= 1.11 and [libsodium].
+Fairly simple. You'll need [Rust] >= 1.12.1 and [libsodium].
 
 When you'll have deps, build debug version with
 ```bash

README.md

@@ -79,7 +79,7 @@ fn main() {
     let mut buf = [0; MAX_UDP_PACKET_SIZE];
 
     // and wait for the answer
-    let (mut bytes, mut sender);
+    let (bytes, sender);
     loop {
         match socket.recv_from(&mut buf) {
             Ok(Some((b, s))) => {
@@ -128,6 +128,8 @@ fn main() {
 #[macro_use]
 extern crate log;
 extern crate mio;
+// for Zero trait
+extern crate num_traits;
 extern crate sodiumoxide;
 
 

src/lib.rs

@@ -19,6 +19,7 @@
 
 //! Functions for binary IO.
 
+use num_traits::identities::Zero;
 
 /// Serialization into bytes.
 pub trait ToBytes {
@@ -131,6 +132,14 @@ pub trait FromBytes: Sized {
 }
 
 
+/// Append `0`s to given bytes up to `len`. Panics if `len` is smaller than
+/// padded `Vec`.
+pub fn append_zeros<T: Clone + Zero>(v: &mut Vec<T>, len: usize) {
+    let l = v.len();
+    v.append(&mut vec![T::zero(); len - l]);
+}
+
+
 // TODO: refactor ↓ using macros?
 
 /// Safely cast `[u8; 2]` to `u16` using shift+or.

src/toxcore/binary_io.rs

@@ -647,13 +647,15 @@ pub struct FriendState {
 pub const FRIENDSTATEBYTES: usize = 1      // "Status"
                                   + PUBLICKEYBYTES
 /* Friend request message      */ + REQUEST_MSG_LEN
+/* padding1                    */ + 1
 /* actual size of FR message   */ + 2
 /* Name;                       */ + NAME_LEN
 /* actual size of Name         */ + 2
 /* Status msg;                 */ + STATUS_MSG_LEN
+/* padding2                    */ + 1
 /* actual size of status msg   */ + 2
 /* UserStatus                  */ + 1
-/* padding                     */ + 3
+/* padding3                    */ + 3
 /* only used for sending FR    */ + NOSPAMBYTES
 /* last time seen              */ + 8;
 
@@ -667,27 +669,32 @@ impl FromBytes for FriendState {
 
         let Parsed(pk, bytes) = try!(PublicKey::parse_bytes(bytes));
 
-        // supply length
+        // supply length and number of bytes that need to be padded
+        // if no padding needed, supply `0`
         // TODO: refactor?
-        fn get_bytes(bytes: &[u8], len: usize) -> ParseResult<Vec<u8>> {
+        fn get_bytes(bytes: &[u8], len: usize, pad: usize)
+            -> ParseResult<Vec<u8>>
+        {
             let str_len = u16::from_be(array_to_u16(
-                        &[bytes[len], bytes[len+1]])) as usize;
+                        &[bytes[len+pad], bytes[len+pad+1]])) as usize;
             if str_len > len {
                 return parse_error!("Value demands {} bytes when it is \
                     supposed to take {}!", str_len, len)
             }
 
-            Ok(Parsed(bytes[..str_len].to_vec(), &bytes[len+2..]))
+            Ok(Parsed(bytes[..str_len].to_vec(), &bytes[len+pad+2..]))
         };
 
-        let Parsed(fr_msg, bytes) = try!(get_bytes(bytes, REQUEST_MSG_LEN));
+        let Parsed(fr_msg, bytes) = try!(get_bytes(bytes, REQUEST_MSG_LEN, 1));
 
         // TODO: refactor?
-        let Parsed(name_bytes, bytes) = try!(get_bytes(bytes, NAME_LEN));
+        let Parsed(name_bytes, bytes) = try!(get_bytes(bytes, NAME_LEN, 0));
         let name = Name(name_bytes);
 
         // TODO: refactor?
-        let Parsed(status_msg_bytes, bytes) = try!(get_bytes(bytes, STATUS_MSG_LEN));
+        let Parsed(status_msg_bytes, bytes) = try!(
+            get_bytes(bytes, STATUS_MSG_LEN, 1)
+        );
         let status_msg = StatusMsg(status_msg_bytes);
 
         let Parsed(user_status, bytes) = try!(UserStatus::parse_bytes(bytes));
@@ -717,13 +724,12 @@ impl FromBytes for FriendState {
 
 impl ToBytes for FriendState {
     fn to_bytes(&self) -> Vec<u8> {
-        // extend vec with all contents of slice and padd with `0`s up to `len`
+        // extend vec with all contents of slice and pad with `0`s up to `len`
         // assume that Vec isn't too big for fr_msg
         fn ext_vec(vec: &mut Vec<u8>, slice: &[u8], len: usize) {
-            vec.extend_from_slice(slice);
-            for _ in 0..(len - slice.len()) {
-                vec.push(0);
-            }
+            let mut to_add = slice.to_vec();
+            append_zeros(&mut to_add, len);
+            vec.append(&mut to_add);
         }
 
         let len_to_u16be = |len| u16_to_array((len as u16).to_be());
@@ -736,25 +742,29 @@ impl ToBytes for FriendState {
         // pk
         result.extend_from_slice(&self.pk.0);
 
-        // friend request msg and its length
+        // friend request msg..
         ext_vec(&mut result, &self.fr_msg, REQUEST_MSG_LEN);
+        // padding
+        result.push(0);
+        // .. and its length
         result.extend_from_slice(&len_to_u16be(self.fr_msg.len()));
 
         // name and its length
         ext_vec(&mut result, &self.name.0, NAME_LEN);
         result.extend_from_slice(&len_to_u16be(self.name.0.len()));
 
-        // status msg and its length
+        // status msg ..
         ext_vec(&mut result, &self.status_msg.0, STATUS_MSG_LEN);
+        // padding
+        result.push(0);
+        // .. and its length
         result.extend_from_slice(&len_to_u16be(self.status_msg.0.len()));
 
         // UserStatus
         result.push(self.user_status as u8);
 
         // padding
-        for _ in 0..3 {
-            result.push(0);
-        }
+        append_zeros(&mut result, FRIENDSTATEBYTES - 12);
 
         // NoSpam
         result.extend_from_slice(&self.nospam.0);
@@ -1414,13 +1424,14 @@ fn friend_state_parse_bytes_test() {
             let mut bytes = fs_bytes.clone();
             // TODO: change to inclusive range (`...`) once gets stabilised
             //       rust #28237
-            for b in 5..u8::max_value() {
+            for b in 5.. {
                 bytes[0] = b;
                 assert_error(&bytes, &format!("Unknown FriendStatus: {}", b));
+                if b == u8::max_value() { break; }
             }
         }
 
-        const FR_MSG_LEN_POS: usize = 1 + PUBLICKEYBYTES + REQUEST_MSG_LEN;
+        const FR_MSG_LEN_POS: usize = 1 + PUBLICKEYBYTES + REQUEST_MSG_LEN + 1;
         { // friend request message lenght check
             let mut bytes = fs_bytes.clone();
             for i in (REQUEST_MSG_LEN+1)..2500 { // too slow with bigger ranges
@@ -1446,7 +1457,8 @@ fn friend_state_parse_bytes_test() {
             }
         }
 
-        const STATUS_MSG_LEN_POS: usize = NAME_LEN_POS + STATUS_MSG_LEN + 2;
+        // padding + bytes containing length
+        const STATUS_MSG_LEN_POS: usize = NAME_LEN_POS + STATUS_MSG_LEN + 3;
         { // friend name lenght check
             let mut bytes = fs_bytes.clone();
             for i in (STATUS_MSG_LEN+1)..2500 { // too slow with bigger ranges

src/toxcore/state_format/old.rs

@@ -19,7 +19,8 @@
 
 //! Tests for `binary_io` module.
 
-use super::quickcheck::quickcheck;
+use num_traits::identities::Zero;
+use super::quickcheck::{quickcheck, TestResult};
 
 use toxcore::binary_io::*;
 
@@ -36,6 +37,49 @@ fn u64_to_array_and_back(num: u64) {
     assert_eq!(num, array_to_u64(&u64_to_array(num)));
 }
 
+
+// append_zeros()
+
+macro_rules! test_append_zeros_for {
+    ($($num:ty, $tname_f:ident, $tname_p:ident),+) => ($(
+        #[test]
+        #[should_panic]
+        fn $tname_f() {
+            fn with_bytes(bytes: Vec<$num>, len: usize) -> TestResult {
+                if bytes.len() > len {
+                    // this should nicely panic
+                    append_zeros(&mut bytes.clone(), len);
+                }
+                TestResult::discard()
+            }
+            quickcheck(with_bytes as fn(Vec<$num>, usize) -> TestResult);
+        }
+
+        #[test]
+        fn $tname_p() {
+            fn with_bytes(bytes: Vec<$num>, len: usize) -> TestResult {
+                if bytes.len() > len {
+                    return TestResult::discard()
+                }
+
+                let mut bc = bytes.clone();
+                append_zeros(&mut bc, len);
+                assert_eq!(bytes[..], bc[..bytes.len()]);
+                assert_eq!(&vec![Zero::zero(); len - bytes.len()] as &[$num],
+                           &bc[bytes.len()..]);
+                TestResult::passed()
+            }
+            quickcheck(with_bytes as fn(Vec<$num>, usize) -> TestResult);
+        }
+    )+)
+}
+test_append_zeros_for!(
+    u8, append_zeros_test_u8_fail, append_zeros_test_u8_pass,
+    u16, append_zeros_test_u16_fail, append_zeros_test_u16_pass,
+    u32, append_zeros_test_u32_fail, append_zeros_test_u32_pass,
+    u64, append_zeros_test_u64_fail, append_zeros_test_u64_pass
+);
+
 #[test]
 fn array_to_u16_test() {
     assert_eq!(array_to_u16(&[0, 0]), 0);