-
Notifications
You must be signed in to change notification settings - Fork 14
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
Txt long values #23
Txt long values #23
Conversation
1c305db
to
9c4e635
Compare
@@ -25,8 +25,10 @@ impl<'a> CharacterString<'a> { | |||
if data.len() > MAX_CHARACTER_STRING_LENGTH { | |||
return Err(SimpleDnsError::InvalidCharacterString); | |||
} | |||
|
|||
Ok(Self { data }) | |||
match String::from_utf8(data.clone().into_owned()) { |
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.
Check for valid utf-8 (and add a test for it), to be able to unwrap at line 44 safely.
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 this check can't be used here, because a character string is not necessarily valid utf-8. If you check this section of the RFC-1035, it says that character string must be treated as binary data.
Since this library can receive records from other libraries, the safest place to check for a valid utf8 string is in the ''Into String" function, because it wouldn't impose valid utf8 when using the library to interface with something else.
Thank you very much for the PR. I have added some suggestions and comments regarding compatibility. BTW, sorry for taking this long to reply, I've missed the email and only saw it today |
@@ -25,8 +25,10 @@ impl<'a> CharacterString<'a> { | |||
if data.len() > MAX_CHARACTER_STRING_LENGTH { | |||
return Err(SimpleDnsError::InvalidCharacterString); | |||
} | |||
|
|||
Ok(Self { data }) | |||
match String::from_utf8(data.clone().into_owned()) { |
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 this check can't be used here, because a character string is not necessarily valid utf-8. If you check this section of the RFC-1035, it says that character string must be treated as binary data.
Since this library can receive records from other libraries, the safest place to check for a valid utf8 string is in the ''Into String" function, because it wouldn't impose valid utf8 when using the library to interface with something else.
simple-dns/src/dns/rdata/txt.rs
Outdated
}, | ||
None => continue, | ||
}; | ||
let full_string: String = (*self).clone().into(); |
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 it is better to create a new function, maybe long_attributes
, rather than changing the existing functionality, because there may be people relying on the current behavior
simple-dns/src/dns/rdata/txt.rs
Outdated
impl<'a> TryFrom<String> for TXT<'a> { | ||
type Error = crate::SimpleDnsError; | ||
|
||
fn try_from(value: String) -> Result<Self, Self::Error> { | ||
let mut txt = TXT::new(); | ||
|
||
let mut start_index = 0; | ||
let full_length = value.len(); | ||
|
||
while start_index < full_length { | ||
let end_index = (start_index + MAX_CHARACTER_STRING_LENGTH).min(full_length); | ||
|
||
let slice = &value[start_index..end_index]; | ||
txt.add_char_string(slice.to_string().try_into()?); | ||
|
||
start_index = end_index; | ||
} | ||
|
||
Ok(txt) | ||
} | ||
} |
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.
This can be simplified if you implement for &str
, and it would also avoid cloning the data
impl<'a> TryFrom<String> for TXT<'a> { | |
type Error = crate::SimpleDnsError; | |
fn try_from(value: String) -> Result<Self, Self::Error> { | |
let mut txt = TXT::new(); | |
let mut start_index = 0; | |
let full_length = value.len(); | |
while start_index < full_length { | |
let end_index = (start_index + MAX_CHARACTER_STRING_LENGTH).min(full_length); | |
let slice = &value[start_index..end_index]; | |
txt.add_char_string(slice.to_string().try_into()?); | |
start_index = end_index; | |
} | |
Ok(txt) | |
} | |
} | |
impl<'a> TryFrom<&'a str> for TXT<'a> { | |
type Error = crate::SimpleDnsError; | |
fn try_from(value: &'a str) -> Result<Self, Self::Error> { | |
let mut txt = TXT::new(); | |
for v in value.as_bytes().chunks(MAX_CHARACTER_STRING_LENGTH) { | |
txt.add_char_string(CharacterString::new(v)?); | |
} | |
Ok(txt) | |
} | |
} |
simple-dns/src/dns/rdata/txt.rs
Outdated
impl<'a> From<TXT<'a>> for String { | ||
fn from(val: TXT<'a>) -> Self { | ||
val.strings | ||
.into_iter() | ||
.map(|s| s.into()) | ||
.collect::<Vec<String>>() | ||
.join("") | ||
} | ||
} |
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.
Here would be the proper place to check for a valid utf8 string, something like this
impl<'a> From<TXT<'a>> for String { | |
fn from(val: TXT<'a>) -> Self { | |
val.strings | |
.into_iter() | |
.map(|s| s.into()) | |
.collect::<Vec<String>>() | |
.join("") | |
} | |
} | |
impl<'a> TryFrom<TXT<'a>> for String { | |
type Error = std::string::FromUtf8Error; | |
fn try_from(val: TXT<'a>) -> Result<Self, Self::Error> { | |
let bytes = val | |
.strings | |
.into_iter() | |
.fold(Vec::with_capacity(val.len()), |mut acc, val| { | |
acc.extend(val.data.as_ref()); | |
acc | |
}); | |
String::from_utf8(bytes) | |
} | |
} | |
@balliegojr Thanks for the feedback, learned a lot, and surprisingly managed to apply the suggested changes without much trouble or adding even more cloning :) |
impl<'a> From<CharacterString<'a>> for String { | ||
fn from(val: CharacterString<'a>) -> Self { | ||
String::from_utf8(val.data.into_owned()).unwrap() | ||
} | ||
} | ||
|
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.
Can you change this to a TryFrom
implementaiton? Since it can be called in a CharacterString that is not valid utf8
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.
done
closes #22
Two changes:
txt.attributes()
operates on the entire value, and respects;
seperators.