From edb99eded624f5129e9af7183c3cc814c95cf63c Mon Sep 17 00:00:00 2001 From: Roman Borschel Date: Wed, 18 Nov 2020 12:03:07 +0100 Subject: [PATCH] [multistream-select] Fix `ls` response encoding/decoding (spec-compliance). (#1811) * Fix ls response encoding/decoding. Thereby remove the now unnecessary arbitrary protocol name length limit. Since it an 'ls' response is always terminated with a dedicated newline (and thus ends with two newlines), an 'ls' response with a single protocol can be disambiguated from a single protocol response by this additional newline. * More commentary * Update versions and changelogs. * Resolve remaining conflict. * Permit empty ls responses, as before. --- CHANGELOG.md | 4 ++ Cargo.toml | 2 +- core/CHANGELOG.md | 2 + core/Cargo.toml | 2 +- misc/multistream-select/CHANGELOG.md | 6 ++ misc/multistream-select/Cargo.toml | 2 +- misc/multistream-select/src/protocol.rs | 75 +++++++++++++------------ 7 files changed, 54 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b56f8cf693b..1e408bd9d46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ - [`parity-multiaddr` CHANGELOG](misc/multiaddr/CHANGELOG.md) - [`libp2p-core-derive` CHANGELOG](misc/core-derive/CHANGELOG.md) +# Version 0.31.0 [unreleased] + +- Update `multistream-select` and all dependent crates. + # Version 0.30.1 [2020-11-11] - Update `libp2p-plaintext`. diff --git a/Cargo.toml b/Cargo.toml index 1342ae8c11b..2c87e40cf61 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ name = "libp2p" edition = "2018" description = "Peer-to-peer networking library" -version = "0.30.1" +version = "0.31.0" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 837c2221ad5..724c55a0cda 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -2,6 +2,8 @@ - Update `multihash`. +- Update `multistream-select`. + # 0.24.0 [2020-11-09] - Remove `ConnectionInfo` trait and replace it with `PeerId` diff --git a/core/Cargo.toml b/core/Cargo.toml index 5e8724cd32c..bd209e2301f 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -23,7 +23,7 @@ libsecp256k1 = { version = "0.3.1", optional = true } log = "0.4" multiaddr = { package = "parity-multiaddr", version = "0.9.4", path = "../misc/multiaddr" } multihash = { version = "0.13", default-features = false, features = ["std", "multihash-impl", "identity", "sha2"] } -multistream-select = { version = "0.8.5", path = "../misc/multistream-select" } +multistream-select = { version = "0.9.0", 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 ac24466f6a9..e61fa9b9d68 100644 --- a/misc/multistream-select/CHANGELOG.md +++ b/misc/multistream-select/CHANGELOG.md @@ -1,3 +1,9 @@ +# 0.9.0 [unreleased] + +- Fix the encoding and decoding of `ls` responses to + be spec-compliant and interoperable with other implementations. + For a clean upgrade, `0.8.4` must already be deployed. + # 0.8.5 [2020-11-09] - During negotiation do not interpret EOF error as an IO error, but instead as a diff --git a/misc/multistream-select/Cargo.toml b/misc/multistream-select/Cargo.toml index d8e3fa167af..7482fc986a1 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.5" +version = "0.9.0" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" diff --git a/misc/multistream-select/src/protocol.rs b/misc/multistream-select/src/protocol.rs index c15ddbc098d..5248247f7ce 100644 --- a/misc/multistream-select/src/protocol.rs +++ b/misc/multistream-select/src/protocol.rs @@ -35,18 +35,6 @@ use unsigned_varint as uvi; /// The maximum number of supported protocols that can be processed. const MAX_PROTOCOLS: usize = 1000; -/// The maximum length (in bytes) of a protocol name. -/// -/// This limit is necessary in order to be able to unambiguously parse -/// response messages without knowledge of the corresponding request. -/// 140 comes about from 3 * 47 = 141, where 47 is the ascii/utf8 -/// encoding of the `/` character and an encoded protocol name is -/// at least 3 bytes long (uvi-length followed by `/` and `\n`). -/// Hence a protocol list response message with 47 protocols is at least -/// 141 bytes long and thus such a response cannot be mistaken for a -/// single protocol response. See `Message::decode`. -const MAX_PROTOCOL_LEN: usize = 140; - /// The encoded form of a multistream-select 1.0.0 header message. const MSG_MULTISTREAM_1_0: &[u8] = b"/multistream/1.0.0\n"; /// The encoded form of a multistream-select 1.0.0 header message. @@ -131,7 +119,7 @@ impl TryFrom for Protocol { type Error = ProtocolError; fn try_from(value: Bytes) -> Result { - if !value.as_ref().starts_with(b"/") || value.len() > MAX_PROTOCOL_LEN { + if !value.as_ref().starts_with(b"/") { return Err(ProtocolError::InvalidProtocol) } Ok(Protocol(value)) @@ -190,7 +178,7 @@ impl Message { let len = p.0.as_ref().len() + 1; // + 1 for \n dest.reserve(len); dest.put(p.0.as_ref()); - dest.put(&b"\n"[..]); + dest.put_u8(b'\n'); Ok(()) } Message::ListProtocols => { @@ -200,14 +188,15 @@ impl Message { } Message::Protocols(ps) => { let mut buf = uvi::encode::usize_buffer(); - let mut out_msg = Vec::from(uvi::encode::usize(ps.len(), &mut buf)); + let mut encoded = Vec::with_capacity(ps.len()); for p in ps { - out_msg.extend(uvi::encode::usize(p.0.as_ref().len() + 1, &mut buf)); // +1 for '\n' - out_msg.extend_from_slice(p.0.as_ref()); - out_msg.push(b'\n') + encoded.extend(uvi::encode::usize(p.0.as_ref().len() + 1, &mut buf)); // +1 for '\n' + encoded.extend_from_slice(p.0.as_ref()); + encoded.push(b'\n') } - dest.reserve(out_msg.len()); - dest.put(out_msg.as_ref()); + encoded.push(b'\n'); + dest.reserve(encoded.len()); + dest.put(encoded.as_ref()); Ok(()) } Message::NotAvailable => { @@ -228,11 +217,6 @@ impl Message { return Ok(Message::Header(Version::V1)) } - if msg.get(0) == Some(&b'/') && msg.last() == Some(&b'\n') && msg.len() <= MAX_PROTOCOL_LEN { - let p = Protocol::try_from(msg.split_to(msg.len() - 1))?; - return Ok(Message::Protocol(p)); - } - if msg == MSG_PROTOCOL_NA { return Ok(Message::NotAvailable); } @@ -241,21 +225,40 @@ impl Message { return Ok(Message::ListProtocols) } - // At this point, it must be a varint number of protocols, i.e. - // a `Protocols` message. - let (num_protocols, mut remaining) = uvi::decode::usize(&msg)?; - if num_protocols > MAX_PROTOCOLS { - return Err(ProtocolError::TooManyProtocols) + // If it starts with a `/`, ends with a line feed without any + // other line feeds in-between, it must be a protocol name. + if msg.get(0) == Some(&b'/') && msg.last() == Some(&b'\n') && + !msg[.. msg.len() - 1].contains(&b'\n') + { + let p = Protocol::try_from(msg.split_to(msg.len() - 1))?; + return Ok(Message::Protocol(p)); } - let mut protocols = Vec::with_capacity(num_protocols); - for _ in 0 .. num_protocols { - let (len, rem) = uvi::decode::usize(remaining)?; - if len == 0 || len > rem.len() || rem[len - 1] != b'\n' { + + // At this point, it must be an `ls` response, i.e. one or more + // length-prefixed, newline-delimited protocol names. + let mut protocols = Vec::new(); + let mut remaining: &[u8] = &msg; + loop { + // A well-formed message must be terminated with a newline. + if remaining == &[b'\n'] { + break + } else if protocols.len() == MAX_PROTOCOLS { + return Err(ProtocolError::TooManyProtocols) + } + + // Decode the length of the next protocol name and check that + // it ends with a line feed. + let (len, tail) = uvi::decode::usize(remaining)?; + if len == 0 || len > tail.len() || tail[len - 1] != b'\n' { return Err(ProtocolError::InvalidMessage) } - let p = Protocol::try_from(Bytes::copy_from_slice(&rem[.. len - 1]))?; + + // Parse the protocol name. + let p = Protocol::try_from(Bytes::copy_from_slice(&tail[.. len - 1]))?; protocols.push(p); - remaining = &rem[len ..] + + // Skip ahead to the next protocol. + remaining = &tail[len ..]; } return Ok(Message::Protocols(protocols));