From f0a15e9b6f704970ee5c9f12dd4704de5e1b37ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Frosteg=C3=A5rd?= Date: Mon, 18 Oct 2021 02:10:39 +0200 Subject: [PATCH] aquatic_udp: improve request parse errors, send less error responses --- Cargo.lock | 1 + aquatic_udp/src/lib/network.rs | 21 ++--- aquatic_udp_protocol/Cargo.toml | 1 + aquatic_udp_protocol/src/request.rs | 122 +++++++++++++++------------- 4 files changed, 78 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f1dcab0..c8f0e2c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -216,6 +216,7 @@ name = "aquatic_udp_protocol" version = "0.1.0" dependencies = [ "byteorder", + "either", "quickcheck", "quickcheck_macros", ] diff --git a/aquatic_udp/src/lib/network.rs b/aquatic_udp/src/lib/network.rs index 82d1117..0ce505d 100644 --- a/aquatic_udp/src/lib/network.rs +++ b/aquatic_udp/src/lib/network.rs @@ -216,21 +216,18 @@ fn read_requests( } } Err(err) => { - ::log::debug!("request_from_bytes error: {:?}", err); + ::log::debug!("Request::from_bytes error: {:?}", err); - if let Some(transaction_id) = err.transaction_id { - let opt_message = if err.error.is_some() { - Some("Parse error".into()) - } else if let Some(message) = err.message { - Some(message.into()) - } else { - None - }; - - if let Some(message) = opt_message { + if let RequestParseError::Sendable { + connection_id, + transaction_id, + err, + } = err + { + if connections.contains_key(&ConnectionKey::new(connection_id, src)) { let response = ErrorResponse { transaction_id, - message, + message: err.right_or("Parse error").into(), }; local_responses.push((response.into(), src)); diff --git a/aquatic_udp_protocol/Cargo.toml b/aquatic_udp_protocol/Cargo.toml index 12d11ce..3cf3f43 100644 --- a/aquatic_udp_protocol/Cargo.toml +++ b/aquatic_udp_protocol/Cargo.toml @@ -9,6 +9,7 @@ repository = "https://github.com/greatest-ape/aquatic" [dependencies] byteorder = "1" +either = "1" [dev-dependencies] quickcheck = "1.0" diff --git a/aquatic_udp_protocol/src/request.rs b/aquatic_udp_protocol/src/request.rs index d88ad8d..8d74196 100644 --- a/aquatic_udp_protocol/src/request.rs +++ b/aquatic_udp_protocol/src/request.rs @@ -3,6 +3,7 @@ use std::io::{self, Cursor, Read, Write}; use std::net::Ipv4Addr; use byteorder::{NetworkEndian, ReadBytesExt, WriteBytesExt}; +use either::Either; use super::common::*; @@ -67,32 +68,40 @@ pub struct ScrapeRequest { } #[derive(Debug)] -pub struct RequestParseError { - pub transaction_id: Option, - pub message: Option, - pub error: Option, +pub enum RequestParseError { + Sendable { + connection_id: ConnectionId, + transaction_id: TransactionId, + err: Either, + }, + Unsendable { + err: Either, + }, } impl RequestParseError { - pub fn new(err: io::Error, transaction_id: i32) -> Self { - Self { - transaction_id: Some(TransactionId(transaction_id)), - message: None, - error: Some(err), + pub fn sendable_io(err: io::Error, connection_id: i64, transaction_id: i32) -> Self { + Self::Sendable { + connection_id: ConnectionId(connection_id), + transaction_id: TransactionId(transaction_id), + err: Either::Left(err), } } - pub fn io(err: io::Error) -> Self { - Self { - transaction_id: None, - message: None, - error: Some(err), + pub fn sendable_text(text: &'static str, connection_id: i64, transaction_id: i32) -> Self { + Self::Sendable { + connection_id: ConnectionId(connection_id), + transaction_id: TransactionId(transaction_id), + err: Either::Right(text), } } - pub fn text(transaction_id: i32, message: &str) -> Self { - Self { - transaction_id: Some(TransactionId(transaction_id)), - message: Some(message.to_string()), - error: None, + pub fn unsendable_io(err: io::Error) -> Self { + Self::Unsendable { + err: Either::Left(err), + } + } + pub fn unsendable_text(text: &'static str) -> Self { + Self::Unsendable { + err: Either::Right(text), } } } @@ -171,13 +180,13 @@ impl Request { let connection_id = cursor .read_i64::() - .map_err(RequestParseError::io)?; + .map_err(RequestParseError::unsendable_io)?; let action = cursor .read_i32::() - .map_err(RequestParseError::io)?; + .map_err(RequestParseError::unsendable_io)?; let transaction_id = cursor .read_i32::() - .map_err(RequestParseError::io)?; + .map_err(RequestParseError::unsendable_io)?; match action { // Connect @@ -188,8 +197,7 @@ impl Request { }) .into()) } else { - Err(RequestParseError::text( - transaction_id, + Err(RequestParseError::unsendable_text( "Protocol identifier missing", )) } @@ -201,39 +209,39 @@ impl Request { let mut peer_id = [0; 20]; let mut ip = [0; 4]; - cursor - .read_exact(&mut info_hash) - .map_err(|err| RequestParseError::new(err, transaction_id))?; - cursor - .read_exact(&mut peer_id) - .map_err(|err| RequestParseError::new(err, transaction_id))?; + cursor.read_exact(&mut info_hash).map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + cursor.read_exact(&mut peer_id).map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; - let bytes_downloaded = cursor - .read_i64::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; - let bytes_left = cursor - .read_i64::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; - let bytes_uploaded = cursor - .read_i64::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; - let event = cursor - .read_i32::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; + let bytes_downloaded = cursor.read_i64::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + let bytes_left = cursor.read_i64::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + let bytes_uploaded = cursor.read_i64::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + let event = cursor.read_i32::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; - cursor - .read_exact(&mut ip) - .map_err(|err| RequestParseError::new(err, transaction_id))?; + cursor.read_exact(&mut ip).map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; - let key = cursor - .read_u32::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; - let peers_wanted = cursor - .read_i32::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; - let port = cursor - .read_u16::() - .map_err(|err| RequestParseError::new(err, transaction_id))?; + let key = cursor.read_u32::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + let peers_wanted = cursor.read_i32::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; + let port = cursor.read_u16::().map_err(|err| { + RequestParseError::sendable_io(err, connection_id, transaction_id) + })?; let opt_ip = if ip == [0; 4] { None @@ -277,7 +285,11 @@ impl Request { .into()) } - _ => Err(RequestParseError::text(transaction_id, "Invalid action")), + _ => Err(RequestParseError::sendable_text( + "Invalid action", + connection_id, + transaction_id, + )), } } }