Handle truncated responses and standardise netcode

This commit adds a new network error variant: TruncatedResponse, which gets returned when a read call yields 0 bytes. This used to be an outright panic, but it was possible to trigger this by pointing dog to a non-DNS TCP server that doesn't send anything back when you give it a DNS request.

The TLS transport is meant to perform the same length-prefix wrangling as the TCP transport, and sure enough, an encrypted non-DNS TCP server resulted in the same thing. Some functions have been extracted to allow for code sharing. This resulted in the buffer being put on the heap, rather than on the stack, so we can return it.

Some more general work was also done to make the four main transport types more consistent with each other.

dns-transport/src/error.rs

@@ -9,6 +9,10 @@ pub enum Error {
     /// There was a problem with the network making a TCP or UDP request.
     NetworkError(std::io::Error),
 
+    /// Not enough information was received from the server before a `read`
+    /// call returned zero bytes.
+    TruncatedResponse,
+
     /// There was a problem making a TLS request.
     #[cfg(feature="tls")]
     TlsError(native_tls::Error),

dns-transport/src/https.rs

@@ -57,9 +57,10 @@ impl Transport for HttpsTransport {
         info!("Opening TLS socket to {:?}", domain);
         let stream = TcpStream::connect(format!("{}:443", domain))?;
         let mut stream = connector.connect(domain, stream)?;
+        debug!("Connected");
 
         let request_bytes = request.to_bytes().expect("failed to serialise request");
-        let mut bytes = format!("\
+        let mut bytes_to_send = format!("\
             POST {} HTTP/1.1\r\n\
             Host: {}\r\n\
             Content-Type: application/dns-message\r\n\
@@ -67,11 +68,11 @@ impl Transport for HttpsTransport {
             User-Agent: {}\r\n\
             Content-Length: {}\r\n\r\n",
             path, domain, USER_AGENT, request_bytes.len()).into_bytes();
-        bytes.extend(request_bytes);
+        bytes_to_send.extend(request_bytes);
 
-        info!("Sending {:?} bytes of data to {}", bytes.len(), self.url);
-        stream.write_all(&bytes)?;
-        debug!("Sent");
+        info!("Sending {} bytes of data to {:?} over HTTPS", bytes_to_send.len(), self.url);
+        stream.write_all(&bytes_to_send)?;
+        debug!("Wrote all bytes");
 
         info!("Waiting to receive...");
         let mut buf = [0; 4096];
@@ -89,10 +90,10 @@ impl Transport for HttpsTransport {
         }
 
         for header in response.headers {
-            trace!("Header {:?} -> {:?}", header.name, String::from_utf8_lossy(header.value));
+            debug!("Header {:?} -> {:?}", header.name, String::from_utf8_lossy(header.value));
         }
 
-        info!("HTTP body has {} bytes", body.len());
+        debug!("HTTP body has {} bytes", body.len());
         let response = Response::from_bytes(&body)?;
         Ok(response)
     }

dns-transport/src/tcp.rs

@@ -54,6 +54,7 @@ impl TcpTransport {
 
 impl Transport for TcpTransport {
     fn send(&self, request: &Request) -> Result<Response, Error> {
+        info!("Opening TCP stream");
         let mut stream =
             if self.addr.contains(':') {
                 TcpStream::connect(&*self.addr)?
@@ -61,32 +62,61 @@ impl Transport for TcpTransport {
             else {
                 TcpStream::connect((&*self.addr, 53))?
             };
-        info!("Created stream");
+        debug!("Opened");
 
         // The message is prepended with the length when sent over TCP,
         // so the server knows how long it is (RFC 1035 §4.2.2)
-        let mut bytes = request.to_bytes().expect("failed to serialise request");
-        let len_bytes = u16::try_from(bytes.len()).expect("request too long").to_be_bytes();
-        bytes.insert(0, len_bytes[0]);
-        bytes.insert(1, len_bytes[1]);
+        let mut bytes_to_send = request.to_bytes().expect("failed to serialise request");
+        Self::prefix_with_length(&mut bytes_to_send);
 
-        info!("Sending {} bytes of data to {} over TCP", bytes.len(), self.addr);
-
-        let written_len = stream.write(&bytes)?;
+        info!("Sending {} bytes of data to {:?} over TCP", bytes_to_send.len(), self.addr);
+        let written_len = stream.write(&bytes_to_send)?;
         debug!("Wrote {} bytes", written_len);
 
+        let read_bytes = Self::length_prefixed_read(&mut stream)?;
+        let response = Response::from_bytes(&read_bytes)?;
+        Ok(response)
+    }
+}
+
+impl TcpTransport {
+
+    /// Mutate the given byte buffer, prefixing it with its own length as a
+    /// big-endian `u16`.
+    pub(crate) fn prefix_with_length(bytes: &mut Vec<u8>) {
+        let len_bytes = u16::try_from(bytes.len())
+            .expect("request too long")
+            .to_be_bytes();
+
+        bytes.insert(0, len_bytes[0]);
+        bytes.insert(1, len_bytes[1]);
+    }
+
+    /// Reads from the given I/O source as many times as necessary to read a
+    /// length-prefixed stream of bytes. The first two bytes are taken as a
+    /// big-endian `u16` to determine the length. Then, that many bytes are
+    /// read from the source.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if there’s a network error during reading, or not
+    /// enough bytes have been sent.
+    pub(crate) fn length_prefixed_read(stream: &mut impl Read) -> Result<Vec<u8>, Error> {
         info!("Waiting to receive...");
-        let mut buf = [0; 4096];
+
+        let mut buf = vec![0; 4096];
         let mut read_len = stream.read(&mut buf[..])?;
 
         if read_len == 0 {
-            panic!("Received no bytes!");
+            warn!("Received no bytes!");
+            return Err(Error::TruncatedResponse);
         }
         else if read_len == 1 {
             info!("Received one byte of data");
             let second_read_len = stream.read(&mut buf[1..])?;
             if second_read_len == 0 {
-                panic!("Received no bytes the second time!");
+                warn!("Received no bytes the second time!");
+                return Err(Error::TruncatedResponse);
             }
 
             read_len += second_read_len;
@@ -97,8 +127,10 @@ impl Transport for TcpTransport {
 
         let total_len = u16::from_be_bytes([buf[0], buf[1]]);
         if read_len - 2 == usize::from(total_len) {
-            let response = Response::from_bytes(&buf[2 .. read_len])?;
-            return Ok(response);
+            debug!("We have enough bytes");
+            let _ = buf.remove(1);
+            let _ = buf.remove(0);
+            return Ok(buf);
         }
 
         debug!("We need to read {} bytes total", total_len);
@@ -109,13 +141,13 @@ impl Transport for TcpTransport {
             info!("Received further {} bytes of data (of {})", extend_len, total_len);
 
             if read_len == 0 {
-                panic!("Read zero bytes!");
+                warn!("Read zero bytes!");
+                return Err(Error::TruncatedResponse);
             }
 
             combined_buffer.extend(&extend_buf[0 .. extend_len]);
         }
 
-        let response = Response::from_bytes(&combined_buffer)?;
-        Ok(response)
+        Ok(combined_buffer)
     }
 }

dns-transport/src/tls.rs

@@ -1,13 +1,12 @@
 #![cfg_attr(not(feature="tls"), allow(unused))]
 
-use std::convert::TryFrom;
 use std::net::TcpStream;
-use std::io::{Read, Write};
+use std::io::Write;
 
 use log::*;
 
 use dns::{Request, Response};
-use super::{Transport, Error};
+use super::{Transport, Error, TcpTransport};
 
 
 /// The **TLS transport**, which sends DNS wire data using TCP through an
@@ -63,27 +62,22 @@ impl Transport for TlsTransport {
                 TcpStream::connect((&*self.addr, 853))?
             };
 
-        info!("Connecting");
-        let mut stream = connector.connect(self.sni_domain(), stream).unwrap();
+        let domain = self.sni_domain();
+        info!("Connecting using domain {:?}", domain);
+        let mut stream = connector.connect(domain, stream).unwrap();
+        debug!("Connected");
 
-        // As with TCP, we need to prepend the message with its length.
-        let mut bytes = request.to_bytes().expect("failed to serialise request");
-        let len_bytes = u16::try_from(bytes.len()).expect("request too long").to_be_bytes();
-        bytes.insert(0, len_bytes[0]);
-        bytes.insert(1, len_bytes[1]);
+        // The message is prepended with the length when sent over TCP,
+        // so the server knows how long it is (RFC 1035 §4.2.2)
+        let mut bytes_to_send = request.to_bytes().expect("failed to serialise request");
+        TcpTransport::prefix_with_length(&mut bytes_to_send);
 
-        info!("Sending {} bytes of data to {}", bytes.len(), self.addr);
-        stream.write_all(&bytes)?;
-        debug!("Sent");
-
-        info!("Waiting to receive...");
-        let mut buf = [0; 4096];
-        let read_len = stream.read(&mut buf)?;
-
-        // Remember to deal with the length again.
-        info!("Received {} bytes of data", read_len);
-        let response = Response::from_bytes(&buf[2 .. read_len])?;
+        info!("Sending {} bytes of data to {} over TLS", bytes_to_send.len(), self.addr);
+        stream.write_all(&bytes_to_send)?;
+        debug!("Wrote all bytes");
 
+        let read_bytes = TcpTransport::length_prefixed_read(&mut stream)?;
+        let response = Response::from_bytes(&read_bytes)?;
         Ok(response)
     }
 

dns-transport/src/udp.rs

@@ -55,20 +55,20 @@ impl Transport for UdpTransport {
         else {
             socket.connect((&*self.addr, 53))?;
         }
+        debug!("Opened");
 
-        let bytes = request.to_bytes().expect("failed to serialise request");
-        info!("Sending {} bytes of data to {} over UDP", bytes.len(), self.addr);
+        let bytes_to_send = request.to_bytes().expect("failed to serialise request");
 
-        let sent_len = socket.send(&bytes)?;
-        debug!("Sent {} bytes", sent_len);
+        info!("Sending {} bytes of data to {} over UDP", bytes_to_send.len(), self.addr);
+        let written_len = socket.send(&bytes_to_send)?;
+        debug!("Wrote {} bytes", written_len);
 
         info!("Waiting to receive...");
-        let mut buf = vec![0; 1024];
+        let mut buf = vec![0; 4096];
         let received_len = socket.recv(&mut buf)?;
 
         info!("Received {} bytes of data", received_len);
         let response = Response::from_bytes(&buf[.. received_len])?;
-
         Ok(response)
     }
 }

src/output.rs

@@ -508,6 +508,7 @@ pub fn print_error_code(rcode: ErrorCode) {
 fn erroneous_phase(error: &TransportError) -> &'static str {
     match error {
         TransportError::WireError(_)          => "protocol",
+        TransportError::TruncatedResponse     |
         TransportError::NetworkError(_)       => "network",
         #[cfg(feature="tls")]
         TransportError::TlsError(_)           |
@@ -522,6 +523,7 @@ fn erroneous_phase(error: &TransportError) -> &'static str {
 fn error_message(error: TransportError) -> String {
     match error {
         TransportError::WireError(e)          => wire_error_message(e),
+        TransportError::TruncatedResponse     => "Truncated response".into(),
         TransportError::NetworkError(e)       => e.to_string(),
         #[cfg(feature="tls")]
         TransportError::TlsError(e)           => e.to_string(),

xtests/bins.toml

@@ -1,3 +1,5 @@
+# HTTPS
+
 [[cmd]]
 name = "Using a DNS-over-HTTPS server that returns status 500"
 shell = "dog --https @https://eu.httpbin.org/status/500 lookup.dog"
@@ -13,3 +15,24 @@ stdout = { empty = true }
 stderr = { string = "Error [protocol]: Malformed packet: insufficient data" }
 status = 1
 tags = [ 'live', 'httpbin', 'https' ]
+
+
+# TCP
+
+# [[cmd]]
+# name = "Using a TCP server that returns an empty message"
+# shell = "dog --tcp @52.20.16.20:30000 lookup.dog"
+# stdout = { empty = true }
+# stderr = { string = "Error [network]: Truncated response" }
+# status = 1
+# tags = [ 'live', 'tcpbin', 'tcp' ]
+
+# The above test is flaky. It works correctly the first time, but produces a
+# different error message on subsequent runs.
+#
+# The ‘other’ tcpbin can be used to test the truncated response error
+# handling, but it requires waiting 60 seconds for their server to give up and
+# send us a FIN:
+#
+# - dog --tcp bsago.me @tcpbin.com:4242
+# - dog --tls bsago.me @tcpbin.com:4243