misc/multistream-select: Interpretation of EOF as Failed negotiation (#1823)

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.

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.

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.3"
-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"

misc/multistream-select/CHANGELOG.md

@@ -1,3 +1,8 @@
+# 0.8.5 [unreleased]
+
+- 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]
 
 - Temporarily disable the internal selection of "parallel" protocol

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 <admin@parity.io>"]
 license = "MIT"
 repository = "https://github.com/libp2p/rust-libp2p"

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 {

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
@@ -152,10 +152,16 @@ where
                 State::RecvMessage { mut io } => {
                     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())))),
+                        // 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.
+                        //
+                        // 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 };
                             return Poll::Pending;

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) => {},