tls_codec: impl for String#2351
Conversation
franziskuskiefer
left a comment
franziskuskiefer
left a comment
There was a problem hiding this comment.
Thanks.
But we need to pick a way to serialize here and describe it so that it's not used when it's not the right encoding.
| @@ -0,0 +1,224 @@ | |||
| use alloc::string::String; | |||
There was a problem hiding this comment.
Let's add a doc comment on top here that this is not specified in the TLS presentation language. But that it's what's implicitly done all the time. And then say how the string is actually being serialized (just its bytes).
| where | ||
| Self: Sized, | ||
| { | ||
| let bytes = TlsByteVecU8::tls_deserialize(bytes)?; |
There was a problem hiding this comment.
This isn't right. You serialize to variable length encoding.
This goes back to the comment on top. We need to clearly describe how strings are being encoded if we want to provider a general implementation.
There was a problem hiding this comment.
Ohh, I thought this would happen by itself. Alright. It looks like VLByteSlice doesn't implement De/SerializeBytes yet, and I would like to impl those as well for nostd support. Should I do that in this PR or a separate one?
| #[cfg(feature = "std")] | ||
| #[test] | ||
| fn roundtrip_deserialize() { | ||
| let original = String::from("roundtrip test"); |
There was a problem hiding this comment.
Using a longer string here would surface the incompatible serialize/deserialize. e.g. it will with let original = String::from_utf8(vec![0x30; 300]).unwrap();.
There was a problem hiding this comment.
You are right, that fails. I'll add it to ensure this is caught.
|
We discussed the next steps offline: The new module needs more documentation, especially around what the format used here is. There are multiple possible encodings, but here we are focusing on varint-length encoded buffers containing utf8 bytes. I'll open a followup issue for supporting Strings with sequences of fixed-length integers in the length field. Strings backed by arrays (i.e. fixed-length byte buffers) will not be supported. Restricting strings (esp with multibyte characters) to fixed byte lengths seems hard to work with. |
2 participants
This implements tls_codec de/serialization for String and &str.
Fixes #2204