From 028ba34920495b2d38f7f2e7cf927c78385667de Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 5 Nov 2020 13:46:33 +0100 Subject: [PATCH 1/6] misc/multistream-select: Differentiate interpretation of EOF > When a listener rejects a protocol with [`Message::NotAvailable`] and the dialer does not have alternative protocols to propose then the dialer will stop the negotiation and drop the corresponding stream. As a listener interpret an EOF after sending a [`Message::NotAvailable`] as a failed negotiation. Interpret an EOF as an io error in all other cases. --- .../multistream-select/src/listener_select.rs | 48 +++++++++++++------ misc/multistream-select/src/tests.rs | 5 +- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/misc/multistream-select/src/listener_select.rs b/misc/multistream-select/src/listener_select.rs index 2d66d666b7b..823eeece9ac 100644 --- a/misc/multistream-select/src/listener_select.rs +++ b/misc/multistream-select/src/listener_select.rs @@ -82,7 +82,11 @@ where { RecvHeader { io: MessageIO }, SendHeader { io: MessageIO, version: Version }, - RecvMessage { io: MessageIO }, + RecvMessage { + io: MessageIO, + /// Whether the last sent message was a [`Message::NotAvailable`]. + prev_sent_na: bool, + }, SendMessage { io: MessageIO, message: Message, @@ -90,7 +94,9 @@ where }, Flush { io: MessageIO, - protocol: Option + protocol: Option, + /// Whether the last sent message was a [`Message::NotAvailable`]. + prev_sent_na: bool }, Done } @@ -144,20 +150,32 @@ where } *this.state = match version { - Version::V1 => State::Flush { io, protocol: None }, - Version::V1Lazy => State::RecvMessage { io }, + Version::V1 => State::Flush { io, protocol: None, prev_sent_na: false }, + Version::V1Lazy => State::RecvMessage { io, prev_sent_na: false }, } } - State::RecvMessage { mut io } => { + State::RecvMessage { mut io, prev_sent_na } => { let msg = match Pin::new(&mut io).poll_next(cx) { Poll::Ready(Some(Ok(msg))) => msg, - Poll::Ready(None) => - return Poll::Ready(Err(NegotiationError::from( - ProtocolError::IoError( - io::ErrorKind::UnexpectedEof.into())))), + Poll::Ready(None) => { + // When a listener rejects a protocol with [`Message::NotAvailable`] and + // the dialer does not have alternative protocols to propose then the + // dialer will stop the negotiation and drop the corresponding stream. + // As a listener interpret an EOF after sending a + // [`Message::NotAvailable`] as a failed negotiation. Interpret an EOF + // as an io error in all other cases. + let err = if prev_sent_na { + NegotiationError::Failed + } else { + NegotiationError::from(ProtocolError::IoError( + io::ErrorKind::UnexpectedEof.into(), + )) + }; + return Poll::Ready(Err(err)); + } Poll::Pending => { - *this.state = State::RecvMessage { io }; + *this.state = State::RecvMessage { io, prev_sent_na }; return Poll::Pending; } Poll::Ready(Some(Err(err))) => return Poll::Ready(Err(From::from(err))), @@ -203,17 +221,19 @@ where Poll::Ready(Err(err)) => return Poll::Ready(Err(From::from(err))), } + let is_na_msg = matches!(message, Message::NotAvailable); + if let Err(err) = Pin::new(&mut io).start_send(message) { return Poll::Ready(Err(From::from(err))); } - *this.state = State::Flush { io, protocol }; + *this.state = State::Flush { io, protocol, prev_sent_na: is_na_msg }; } - State::Flush { mut io, protocol } => { + State::Flush { mut io, protocol, prev_sent_na } => { match Pin::new(&mut io).poll_flush(cx) { Poll::Pending => { - *this.state = State::Flush { io, protocol }; + *this.state = State::Flush { io, protocol, prev_sent_na }; return Poll::Pending }, Poll::Ready(Ok(())) => { @@ -226,7 +246,7 @@ where let io = Negotiated::completed(io.into_inner()); return Poll::Ready(Ok((protocol, io))) } - None => *this.state = State::RecvMessage { io } + None => *this.state = State::RecvMessage { io, prev_sent_na } } } Poll::Ready(Err(err)) => return Poll::Ready(Err(From::from(err))), diff --git a/misc/multistream-select/src/tests.rs b/misc/multistream-select/src/tests.rs index c5ddd43ea3a..50ab7ded016 100644 --- a/misc/multistream-select/src/tests.rs +++ b/misc/multistream-select/src/tests.rs @@ -85,9 +85,8 @@ fn no_protocol_found() { let protos = vec![b"/proto1", b"/proto2"]; let io = match listener_select_proto(connec, protos).await { Ok((_, io)) => io, - // We don't explicitly check for `Failed` because the client might close the connection when it - // realizes that we have no protocol in common. - Err(_) => return, + Err(NegotiationError::Failed) => return, + Err(NegotiationError::ProtocolError(e)) => panic!("Unexpected protocol error {}", e), }; match io.complete().await { Err(NegotiationError::Failed) => {}, From d74b242cb9030494fc1a8909b0d855655aae944c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 5 Nov 2020 14:08:29 +0100 Subject: [PATCH 2/6] *: Update cargo tomls and changelogs --- core/Cargo.toml | 2 +- misc/multistream-select/CHANGELOG.md | 6 ++++++ misc/multistream-select/Cargo.toml | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/core/Cargo.toml b/core/Cargo.toml index a4cee775e3d..17aa0b7623b 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -22,7 +22,7 @@ libsecp256k1 = { version = "0.3.1", optional = true } log = "0.4" multiaddr = { package = "parity-multiaddr", version = "0.9.2", path = "../misc/multiaddr" } multihash = "0.11.0" -multistream-select = { version = "0.8.4", path = "../misc/multistream-select" } +multistream-select = { version = "0.8.5", path = "../misc/multistream-select" } parking_lot = "0.11.0" pin-project = "1.0.0" prost = "0.6.1" diff --git a/misc/multistream-select/CHANGELOG.md b/misc/multistream-select/CHANGELOG.md index 8e054427ac4..ea44e1de8a2 100644 --- a/misc/multistream-select/CHANGELOG.md +++ b/misc/multistream-select/CHANGELOG.md @@ -1,3 +1,9 @@ +# 0.8.5 [unreleased] + +- Do not interpret EOF error after sending a `NotAvailable` message as an IO + error, but instead as a negotiation error. See + https://github.com/libp2p/rust-libp2p/pull/1823. + # 0.8.4 [2020-10-20] - Temporarily disable the internal selection of "parallel" protocol diff --git a/misc/multistream-select/Cargo.toml b/misc/multistream-select/Cargo.toml index 67efeade0e3..d8e3fa167af 100644 --- a/misc/multistream-select/Cargo.toml +++ b/misc/multistream-select/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "multistream-select" description = "Multistream-select negotiation protocol for libp2p" -version = "0.8.4" +version = "0.8.5" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" From 01b54b891c648df23d8480fef3420239b3158316 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 5 Nov 2020 17:22:59 +0100 Subject: [PATCH 3/6] misc/multistream-select: Always interpret EOF as negotiation failure --- .../multistream-select/src/listener_select.rs | 51 +++++++------------ 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/misc/multistream-select/src/listener_select.rs b/misc/multistream-select/src/listener_select.rs index 823eeece9ac..2d8989ad3b0 100644 --- a/misc/multistream-select/src/listener_select.rs +++ b/misc/multistream-select/src/listener_select.rs @@ -82,11 +82,7 @@ where { RecvHeader { io: MessageIO }, SendHeader { io: MessageIO, version: Version }, - RecvMessage { - io: MessageIO, - /// Whether the last sent message was a [`Message::NotAvailable`]. - prev_sent_na: bool, - }, + RecvMessage { io: MessageIO }, SendMessage { io: MessageIO, message: Message, @@ -94,9 +90,7 @@ where }, Flush { io: MessageIO, - protocol: Option, - /// Whether the last sent message was a [`Message::NotAvailable`]. - prev_sent_na: bool + protocol: Option }, Done } @@ -150,32 +144,23 @@ where } *this.state = match version { - Version::V1 => State::Flush { io, protocol: None, prev_sent_na: false }, - Version::V1Lazy => State::RecvMessage { io, prev_sent_na: false }, + Version::V1 => State::Flush { io, protocol: None }, + Version::V1Lazy => State::RecvMessage { io }, } } - State::RecvMessage { mut io, prev_sent_na } => { + State::RecvMessage { mut io } => { let msg = match Pin::new(&mut io).poll_next(cx) { Poll::Ready(Some(Ok(msg))) => msg, - Poll::Ready(None) => { - // When a listener rejects a protocol with [`Message::NotAvailable`] and - // the dialer does not have alternative protocols to propose then the - // dialer will stop the negotiation and drop the corresponding stream. - // As a listener interpret an EOF after sending a - // [`Message::NotAvailable`] as a failed negotiation. Interpret an EOF - // as an io error in all other cases. - let err = if prev_sent_na { - NegotiationError::Failed - } else { - NegotiationError::from(ProtocolError::IoError( - io::ErrorKind::UnexpectedEof.into(), - )) - }; - return Poll::Ready(Err(err)); - } + // Treat EOF error as [`NegotiationError::Failed`], not as + // [`NegotiationError::ProtocolError`]. This is e.g. important when a + // listener rejects a protocol with [`Message::NotAvailable`] and the dialer + // does not have alternative protocols to propose. Then the dialer will stop + // the negotiation and drop the corresponding stream. As a listener this EOF + // should be interpreted as a failed negotiation. + Poll::Ready(None) => return Poll::Ready(Err(NegotiationError::Failed)), Poll::Pending => { - *this.state = State::RecvMessage { io, prev_sent_na }; + *this.state = State::RecvMessage { io }; return Poll::Pending; } Poll::Ready(Some(Err(err))) => return Poll::Ready(Err(From::from(err))), @@ -221,19 +206,17 @@ where Poll::Ready(Err(err)) => return Poll::Ready(Err(From::from(err))), } - let is_na_msg = matches!(message, Message::NotAvailable); - if let Err(err) = Pin::new(&mut io).start_send(message) { return Poll::Ready(Err(From::from(err))); } - *this.state = State::Flush { io, protocol, prev_sent_na: is_na_msg }; + *this.state = State::Flush { io, protocol }; } - State::Flush { mut io, protocol, prev_sent_na } => { + State::Flush { mut io, protocol } => { match Pin::new(&mut io).poll_flush(cx) { Poll::Pending => { - *this.state = State::Flush { io, protocol, prev_sent_na }; + *this.state = State::Flush { io, protocol }; return Poll::Pending }, Poll::Ready(Ok(())) => { @@ -246,7 +229,7 @@ where let io = Negotiated::completed(io.into_inner()); return Poll::Ready(Ok((protocol, io))) } - None => *this.state = State::RecvMessage { io, prev_sent_na } + None => *this.state = State::RecvMessage { io } } } Poll::Ready(Err(err)) => return Poll::Ready(Err(From::from(err))), From fa67d7b49d3721dcc29a7e9b4b0b7ab39519294d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 5 Nov 2020 17:26:42 +0100 Subject: [PATCH 4/6] misc/multistream-select/CHANGELOG: Update --- misc/multistream-select/CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/misc/multistream-select/CHANGELOG.md b/misc/multistream-select/CHANGELOG.md index ea44e1de8a2..3b8691c14a2 100644 --- a/misc/multistream-select/CHANGELOG.md +++ b/misc/multistream-select/CHANGELOG.md @@ -1,8 +1,7 @@ # 0.8.5 [unreleased] -- Do not interpret EOF error after sending a `NotAvailable` message as an IO - error, but instead as a negotiation error. See - https://github.com/libp2p/rust-libp2p/pull/1823. +- As a listener do not interpret EOF error as an IO error, but instead as a + negotiation error. See https://github.com/libp2p/rust-libp2p/pull/1823. # 0.8.4 [2020-10-20] From adf31c8adda33fb48f40e36b79e27e85a7d9d9fa Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 9 Nov 2020 15:03:51 +0100 Subject: [PATCH 5/6] misc/multistream-select: Fail negotiation for all Poll::Ready(None) --- misc/multistream-select/src/dialer_select.rs | 16 +++++++------ .../multistream-select/src/listener_select.rs | 23 +++++++++++-------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/misc/multistream-select/src/dialer_select.rs b/misc/multistream-select/src/dialer_select.rs index 18509e942b7..38111d99c25 100644 --- a/misc/multistream-select/src/dialer_select.rs +++ b/misc/multistream-select/src/dialer_select.rs @@ -24,7 +24,7 @@ use crate::{Negotiated, NegotiationError}; use crate::protocol::{Protocol, ProtocolError, MessageIO, Message, Version}; use futures::{future::Either, prelude::*}; -use std::{convert::TryFrom as _, io, iter, mem, pin::Pin, task::{Context, Poll}}; +use std::{convert::TryFrom as _, iter, mem, pin::Pin, task::{Context, Poll}}; /// Returns a `Future` that negotiates a protocol on the given I/O stream /// for a peer acting as the _dialer_ (or _initiator_). @@ -235,9 +235,10 @@ where *this.state = SeqState::AwaitProtocol { io, protocol }; return Poll::Pending } - Poll::Ready(None) => - return Poll::Ready(Err(NegotiationError::from( - io::Error::from(io::ErrorKind::UnexpectedEof)))), + // Treat EOF error as [`NegotiationError::Failed`], not as + // [`NegotiationError::ProtocolError`], allowing dropping or closing an I/O + // stream as a permissible way to "gracefully" fail a negotiation. + Poll::Ready(None) => return Poll::Ready(Err(NegotiationError::Failed)), }; match msg { @@ -358,9 +359,10 @@ where *this.state = ParState::RecvProtocols { io }; return Poll::Pending } - Poll::Ready(None) => - return Poll::Ready(Err(NegotiationError::from( - io::Error::from(io::ErrorKind::UnexpectedEof)))), + // Treat EOF error as [`NegotiationError::Failed`], not as + // [`NegotiationError::ProtocolError`], allowing dropping or closing an I/O + // stream as a permissible way to "gracefully" fail a negotiation. + Poll::Ready(None) => return Poll::Ready(Err(NegotiationError::Failed)), }; match &msg { diff --git a/misc/multistream-select/src/listener_select.rs b/misc/multistream-select/src/listener_select.rs index 2d8989ad3b0..04f23228369 100644 --- a/misc/multistream-select/src/listener_select.rs +++ b/misc/multistream-select/src/listener_select.rs @@ -26,7 +26,7 @@ use crate::protocol::{Protocol, ProtocolError, MessageIO, Message, Version}; use futures::prelude::*; use smallvec::SmallVec; -use std::{convert::TryFrom as _, io, iter::FromIterator, mem, pin::Pin, task::{Context, Poll}}; +use std::{convert::TryFrom as _, iter::FromIterator, mem, pin::Pin, task::{Context, Poll}}; /// Returns a `Future` that negotiates a protocol on the given I/O stream /// for a peer acting as the _listener_ (or _responder_). @@ -118,10 +118,10 @@ where return Poll::Ready(Err(ProtocolError::InvalidMessage.into())) }, Poll::Ready(Some(Err(err))) => return Poll::Ready(Err(From::from(err))), - Poll::Ready(None) => - return Poll::Ready(Err(NegotiationError::from( - ProtocolError::IoError( - io::ErrorKind::UnexpectedEof.into())))), + // Treat EOF error as [`NegotiationError::Failed`], not as + // [`NegotiationError::ProtocolError`], allowing dropping or closing an I/O + // stream as a permissible way to "gracefully" fail a negotiation. + Poll::Ready(None) => return Poll::Ready(Err(NegotiationError::Failed)), Poll::Pending => { *this.state = State::RecvHeader { io }; return Poll::Pending @@ -153,11 +153,14 @@ where let msg = match Pin::new(&mut io).poll_next(cx) { Poll::Ready(Some(Ok(msg))) => msg, // Treat EOF error as [`NegotiationError::Failed`], not as - // [`NegotiationError::ProtocolError`]. This is e.g. important when a - // listener rejects a protocol with [`Message::NotAvailable`] and the dialer - // does not have alternative protocols to propose. Then the dialer will stop - // the negotiation and drop the corresponding stream. As a listener this EOF - // should be interpreted as a failed negotiation. + // [`NegotiationError::ProtocolError`], allowing dropping or closing an I/O + // stream as a permissible way to "gracefully" fail a negotiation. + // + // This is e.g. important when a listener rejects a protocol with + // [`Message::NotAvailable`] and the dialer does not have alternative + // protocols to propose. Then the dialer will stop the negotiation and drop + // the corresponding stream. As a listener this EOF should be interpreted as + // a failed negotiation. Poll::Ready(None) => return Poll::Ready(Err(NegotiationError::Failed)), Poll::Pending => { *this.state = State::RecvMessage { io }; From 1ff4801a167e39eec13699fb31d945bf7483869d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 9 Nov 2020 15:09:37 +0100 Subject: [PATCH 6/6] misc/multistream-select/CHANGELOG: Update --- misc/multistream-select/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc/multistream-select/CHANGELOG.md b/misc/multistream-select/CHANGELOG.md index 3b8691c14a2..d81960dfc6d 100644 --- a/misc/multistream-select/CHANGELOG.md +++ b/misc/multistream-select/CHANGELOG.md @@ -1,6 +1,6 @@ # 0.8.5 [unreleased] -- As a listener do not interpret EOF error as an IO error, but instead as a +- During negotiation do not interpret EOF error as an IO error, but instead as a negotiation error. See https://github.com/libp2p/rust-libp2p/pull/1823. # 0.8.4 [2020-10-20]