Repository: aisk/rust-memcache (language: rust)

Issue:
Binary protocol bug
In cases when `ResponseStatus::KeyNotFound`, `ResponseStatus::KeyExists` is returned by the server, there can be a string body after the header denoting the error. Currently we don't read the body and this causes `ClientError`s when using the command after these statuses are returned.

Example:

```
use memcache::Client;
let mut client = Client::connect("memcache://localhost:11211").unwrap();
client.add("key", "value", 0).unwrap();
client.add("key", "value", 0).unwrap();
client.add("key", "value", 0).unwrap();
```

The third `add` will panic here. I think this is also applicable for `append`, `replace` etc. also.

The bug is [here](https://github.com/aisk/rust-memcache/blob/66bd9671df31377f3edf9e4fce7d7aa11615d23f/src/protocol/binary_packet.rs#L106) It should check if  `value_length` in `total_body_length` is `0`.


Discussion / hints:

Fix patch:
diff --git a/src/protocol/binary.rs b/src/protocol/binary.rs
index b44bb2f..231ce5b 100644
--- a/src/protocol/binary.rs
+++ b/src/protocol/binary.rs
@@ -72,7 +72,7 @@ impl BinaryProtocol {
         cas: Option<u64>,
     ) -> Result<(), MemcacheError> {
         self.send_request(opcode, key, value, expiration, cas)?;
-        return binary_packet::parse_header_only_response(&mut self.stream);
+        return binary_packet::parse_response(&mut self.stream);
     }
 
     pub(super) fn version(&mut self) -> Result<String, MemcacheError> {
@@ -95,7 +95,7 @@ impl BinaryProtocol {
         };
         request_header.write(&mut self.stream)?;
         self.stream.flush()?;
-        binary_packet::parse_header_only_response(&mut self.stream)?;
+        binary_packet::parse_response(&mut self.stream)?;
         return Ok(());
     }
 
@@ -110,7 +110,7 @@ impl BinaryProtocol {
         request_header.write(&mut self.stream)?;
         self.stream.write_u32::<BigEndian>(delay)?;
         self.stream.flush()?;
-        binary_packet::parse_header_only_response(&mut self.stream)?;
+        binary_packet::parse_response(&mut self.stream)?;
         return Ok(());
     }
 
@@ -211,7 +211,7 @@ impl BinaryProtocol {
         self.stream.write_all(key.as_bytes())?;
         value.write_to(&mut self.stream)?;
         self.stream.flush()?;
-        return binary_packet::parse_header_only_response(&mut self.stream);
+        return binary_packet::parse_response(&mut self.stream);
     }
 
     pub(super) fn prepend<V: ToMemcacheValue<Stream>>(&mut self, key: &str, value: V) -> Result<(), MemcacheError> {
@@ -229,7 +229,7 @@ impl BinaryProtocol {
         self.stream.write_all(key.as_bytes())?;
         value.write_to(&mut self.stream)?;
         self.stream.flush()?;
-        return binary_packet::parse_header_only_response(&mut self.stream);
+        return binary_packet::parse_response(&mut self.stream);
     }
 
     pub(super) fn delete(&mut self, key: &str) -> Result<bool, MemcacheError> {
diff --git a/src/protocol/binary_packet.rs b/src/protocol/binary_packet.rs
index a3f2eb5..0367b44 100644
--- a/src/protocol/binary_packet.rs
+++ b/src/protocol/binary_packet.rs
@@ -118,8 +118,10 @@ pub fn parse_cas_response<R: io::Read>(stream: &mut R) -> Result<bool, MemcacheE
     }
 }
 
-pub fn parse_header_only_response<R: io::Read>(reader: &mut R) -> Result<(), MemcacheError> {
+pub fn parse_response<R: io::Read>(reader: &mut R) -> Result<(), MemcacheError> {
     let header = PacketHeader::read(reader)?;
+    let mut buffer = vec![0; header.total_body_length as usize];
+    reader.read_exact(buffer.as_mut_slice())?;
     if header.vbucket_id_or_status != ResponseStatus::NoError as u16 {
         return Err(MemcacheError::from(header.vbucket_id_or_status));
     }


Test patch:
diff --git a/tests/tests.rs b/tests/tests.rs
index 64e7c14..ca24dc4 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -57,6 +57,38 @@ fn test() {
     }
 }
 
+#[test]
+fn issue74() {
+    let mut client = memcache::Client::connect("memcache://localhost:12346?tcp_nodelay=true").unwrap();
+    client.delete("issue74").unwrap();
+    client.add("issue74", 1, 0).unwrap();
+
+    match client.add("issue74", 1, 0) {
+        Ok(_) => panic!("Should got an error!"),
+        Err(e) => match e {
+            memcache::MemcacheError::ServerError(e) => assert!(e == 2),
+            _ => panic!("Unexpected error!"),
+        }
+    }
+
+    match client.add("issue74", 1, 0) {
+        Ok(_) => panic!("Should got an error!"),
+        Err(e) => match e {
+            memcache::MemcacheError::ServerError(e) => assert!(e == 2),
+            _ => panic!("Unexpected error!"),
+        }
+    }
+
+    match client.add("issue74", 1, 0) {
+        Ok(_) => panic!("Should got an error!"),
+        Err(e) => match e {
+            memcache::MemcacheError::ServerError(e) => assert!(e == 2),
+            _ => panic!("Unexpected error!"),
+        }
+    }
+}
+
+
 #[test]
 fn udp_test() {
     let urls = vec!["memcache+udp://localhost:22345"];