-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[mdns] - Support for long mDNS names (Bug #1232) #1287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
b68866e
Dead code -- commenting out with a note referencing future implementa…
peat fcef341
Adding "std" feature so that cargo can build in other directories (no…
peat 68d05c8
Permitting `PeerID` to be built from an `Identity` multihash
peat 9fde5e2
The length limit for DNS labels is 63 characters, as per RFC1035
peat 866dcc1
Allocates the vector with capacity for the service name plus addition…
peat 1aa0ae6
Added support for encoding/decoding peer IDs with an encoded length g…
peat da6d6fe
Removing "std" from ring features
peat ced22fe
Retaining MAX_INLINE_KEY_LENGTH with comment about future usage
peat a3ed493
`segment_peer_id` consumes `peer_id` ... plus an early return for IDs…
peat 8546f56
Merge branch 'master' into long-mdns
peat ec7e5d5
Merge branch 'master' into long-mdns
peat 05aee34
Merge branch 'long-mdns' of github.com:peat/rust-libp2p into long-mdns
peat b26b24c
Fixing logic
peat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,9 @@ use libp2p_core::{Multiaddr, PeerId}; | |
| use rand; | ||
| use std::{borrow::Cow, cmp, error, fmt, str, time::Duration}; | ||
|
|
||
| /// Maximum size of a DNS label as per RFC1035 | ||
| const MAX_LABEL_LENGTH: usize = 63; | ||
|
|
||
| /// Decodes a `<character-string>` (as defined by RFC1035) into a `Vec` of ASCII characters. | ||
| // TODO: better error type? | ||
| pub fn decode_character_string(mut from: &[u8]) -> Result<Cow<'_, [u8]>, ()> { | ||
|
|
@@ -117,23 +120,15 @@ pub fn build_query_response( | |
| // TTL for the answer | ||
| append_u32(&mut out, ttl); | ||
|
|
||
| let peer_id_base58 = peer_id.to_base58(); | ||
|
|
||
| // Peer Id. | ||
| let peer_name = format!( | ||
| "{}.{}", | ||
| data_encoding::BASE32_DNSCURVE.encode(&peer_id.into_bytes()), | ||
| str::from_utf8(SERVICE_NAME).expect("SERVICE_NAME is always ASCII") | ||
| ); | ||
| let mut peer_id_bytes = Vec::with_capacity(64); | ||
| append_qname(&mut peer_id_bytes, peer_name.as_bytes()); | ||
| let peer_id_bytes = encode_peer_id(&peer_id); | ||
| debug_assert!(peer_id_bytes.len() <= 0xffff); | ||
| append_u16(&mut out, peer_id_bytes.len() as u16); | ||
| out.extend_from_slice(&peer_id_bytes); | ||
|
|
||
| // The TXT records for answers. | ||
| for addr in addresses { | ||
| let txt_to_send = format!("dnsaddr={}/p2p/{}", addr.to_string(), peer_id_base58); | ||
| let txt_to_send = format!("dnsaddr={}/p2p/{}", addr.to_string(), peer_id.to_base58()); | ||
| let mut txt_to_send_bytes = Vec::with_capacity(txt_to_send.len()); | ||
| append_character_string(&mut txt_to_send_bytes, txt_to_send.as_bytes())?; | ||
| append_txt_record(&mut out, &peer_id_bytes, ttl, Some(&txt_to_send_bytes[..]))?; | ||
|
|
@@ -177,7 +172,7 @@ pub fn build_service_discovery_response(id: u16, ttl: Duration) -> Vec<u8> { | |
|
|
||
| // Service name. | ||
| { | ||
| let mut name = Vec::new(); | ||
| let mut name = Vec::with_capacity(SERVICE_NAME.len() + 2); | ||
| append_qname(&mut name, SERVICE_NAME); | ||
| append_u16(&mut out, name.len() as u16); | ||
| out.extend_from_slice(&name); | ||
|
|
@@ -211,6 +206,40 @@ fn append_u16(out: &mut Vec<u8>, value: u16) { | |
| out.push((value & 0xff) as u8); | ||
| } | ||
|
|
||
| /// If a peer ID is longer than 63 characters, split it into segments to | ||
| /// be compatible with RFC 1035. | ||
| fn segment_peer_id(peer_id: String) -> String { | ||
| // Guard for the most common case | ||
| if peer_id.len() <= MAX_LABEL_LENGTH { return peer_id } | ||
|
|
||
| // This will only perform one allocation except in extreme circumstances. | ||
| let mut out = String::with_capacity(peer_id.len() + 8); | ||
|
|
||
| for (idx, chr) in peer_id.chars().enumerate() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line bothers me a bit. We know the string only contains ASCII characters, so I guess it's fine. I would have preferred using bytes. |
||
| if idx > 0 && idx % MAX_LABEL_LENGTH == 0 { | ||
| out.push('.'); | ||
| } | ||
| out.push(chr); | ||
| } | ||
| out | ||
| } | ||
|
|
||
| /// Combines and encodes a `PeerId` and service name for a DNS query. | ||
| fn encode_peer_id(peer_id: &PeerId) -> Vec<u8> { | ||
| // DNS-safe encoding for the Peer ID | ||
| let raw_peer_id = data_encoding::BASE32_DNSCURVE.encode(&peer_id.as_bytes()); | ||
| // ensure we don't have any labels over 63 bytes long | ||
| let encoded_peer_id = segment_peer_id(raw_peer_id); | ||
| let service_name = str::from_utf8(SERVICE_NAME).expect("SERVICE_NAME is always ASCII"); | ||
| let peer_name = [&encoded_peer_id, service_name].join("."); | ||
|
|
||
| // allocate with a little extra padding for QNAME encoding | ||
| let mut peer_id_bytes = Vec::with_capacity(peer_name.len() + 32); | ||
| append_qname(&mut peer_id_bytes, peer_name.as_bytes()); | ||
|
|
||
| peer_id_bytes | ||
| } | ||
|
|
||
| /// Appends a `QNAME` (as defined by RFC1035) to the `Vec`. | ||
| /// | ||
| /// # Panic | ||
|
|
@@ -223,7 +252,7 @@ fn append_qname(out: &mut Vec<u8>, name: &[u8]) { | |
| debug_assert!(name.is_ascii()); | ||
|
|
||
| for element in name.split(|&c| c == b'.') { | ||
| assert!(element.len() < 256, "Service name has a label too long"); | ||
| assert!(element.len() < 64, "Service name has a label too long"); | ||
| assert_ne!(element.len(), 0, "Service name contains zero length label"); | ||
| out.push(element.len() as u8); | ||
| for chr in element.iter() { | ||
|
|
@@ -367,5 +396,21 @@ mod tests { | |
| assert!(Packet::parse(&query).is_ok()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_segment_peer_id() { | ||
| let str_32 = String::from_utf8(vec![b'x'; 32]).unwrap(); | ||
| let str_63 = String::from_utf8(vec![b'x'; 63]).unwrap(); | ||
| let str_64 = String::from_utf8(vec![b'x'; 64]).unwrap(); | ||
| let str_126 = String::from_utf8(vec![b'x'; 126]).unwrap(); | ||
| let str_127 = String::from_utf8(vec![b'x'; 127]).unwrap(); | ||
|
|
||
| assert_eq!(segment_peer_id(str_32.clone()), str_32); | ||
| assert_eq!(segment_peer_id(str_63.clone()), str_63); | ||
|
|
||
| assert_eq!(segment_peer_id(str_64), [&str_63, "x"].join(".")); | ||
| assert_eq!(segment_peer_id(str_126), [&str_63, str_63.as_str()].join(".")); | ||
| assert_eq!(segment_peer_id(str_127), [&str_63, &str_63, "x"].join(".")); | ||
| } | ||
|
|
||
| // TODO: test limits and errors | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can replace the function's body with something like this:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that
.chunkscan't.join(in 1.38.0) ... I went through several iterations to see if I could make that work elegantly, and ended up with thefor ... inloop for clarity and minimal number of allocations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked further into this as I was curious, I guess not because it really matters in this case. I am sorry if this is just useless noise to the discussion.
Under the assumption that the string only contains ASCII characters, why not do:
Using a for-loop and manually preallocating the right amount of memory seems like an early optimization to me. I have benchmarked this here. Using iterators with
joinallows one to not copy the characters one by one as in the for loop, but instead in chunks (See https://doc.rust-lang.org/src/alloc/slice.rs.html#664).Running the above linked benchmarks in criterion shows that the above would be twice as fast (on my machine) as the for loop. In addition I find it easier to comprehend.
@peat @tomaka what do you think?