Conversation
|
Will rebase when/if #47 lands. |
algesten
force-pushed
the
feature/combine-dtls-fragments
branch
5 times, most recently
from
8970f45 to
6a5f3d2
Compare
algesten
force-pushed
the
feature/combine-dtls-fragments
branch
from
6a5f3d2 to
fdf3420
Compare
|
I believe this is ready to merge now. |
| /// 5. The DTLSMessageHandshakeBody in the message is not a Fragment | ||
| /// 6. The message_seq differs between the fragments. | ||
| /// | ||
| /// Panics if there are more than 50 fragments. |
There was a problem hiding this comment.
(minor) This comment is not correct now, it will return an error (and not panic)
There was a problem hiding this comment.
Yep will fix. Following the logic (if we keep that), this should be error case 7 or something.
| // Unwrap is OK, because we have at least one item (checked above). | ||
| let first_handshake = ordered.iter().next().unwrap(); |
There was a problem hiding this comment.
(minor) The test is not obvious, since it relies on Ordered keeping the same number of items as fragments. A test here (even if redundant) would not be bad to avoid unwrap, and would not cause performance problems.
There was a problem hiding this comment.
Alright. It's an invariant, in the sense that if I got this assumption wrong, there's something more serious wrong (bug). I generally don't like masking those as errors, as it would appear the user provided incorrect data when it's actually a bug in this code.
But happy to change to error.
| .get(*idx) | ||
| .map(|h| h.fragment_offset) | ||
| // Somewhere outside the fragments length. | ||
| .unwrap_or(*idx as u32 + 1000) |
There was a problem hiding this comment.
What is this value? It seems dangerous to return a random offset if the fragment with this ID does not exist.
Should this return an error? Or is this a magic value?
Also, note that Option has a method map_or_else that could be used.
There was a problem hiding this comment.
Yeah. Poor logic here. It's another invariant. idx comes from order which means it's impossible for .get(*idx) to return None. Probably better to change this to a direct array reference fragments[idx].fragment_offset
I fix.
There was a problem hiding this comment.
This is a consequence of not using a sorted list.
Well, even if there is an invariant, it is not described (and a minimum would be to use assert/debug_assert to state it).
| Ok((rest, Some(message))) | ||
| } | ||
|
|
||
| struct Ordered<'a, 'b>([usize; MAX_FRAGMENTS], &'a [DTLSMessageHandshake<'b>]); |
There was a problem hiding this comment.
I am missing the entire logic of this structure, this may require some more explanations.
As far as I understand, the field 0 is the sorted list of indices, the fragments being in the field 1.
Why not create and use directly a list of sorted fragments ? Or, to better deal with lifetimes, references to fragments ?
You would just have to iterate the list of fragments, and insert them in a sorted structure (list, tree, etc).
There was a problem hiding this comment.
The goal here is to avoid an extra allocation. The user provides &[DTLSMessageHandshake], which could have been received in any order (given this is UDP). The order we want is in the fragment_offset field of each message. Thus, the Ordered iterator iterates the messages without any extra heap allocation.
Personally I think this is better than allocating, but I should provide this motivation as code doc. However if you prefer new Vec, I can make that too.
There was a problem hiding this comment.
Thank you for clarifying. The methods seems globally fine, and there is no need to switch to an allocation.
That said, I think the code needs some comments to be more readable later. This is my main concern.
Also, the fields could be named (for ex indices/fragments) for clarity, and avoid .0 and .1.
| if msg_type != handshake.msg_type { | ||
| // Error case 2 | ||
| return Err(Err::Error(make_error(&*out, ErrorKind::Fail))); | ||
| } | ||
|
|
||
| if handshake.length != length { | ||
| // Error case 3 | ||
| return Err(Err::Error(make_error(&*out, ErrorKind::Fail))); | ||
| } | ||
|
|
||
| if handshake.message_seq != message_seq { | ||
| // Error case 6 | ||
| return Err(Err::Error(make_error(&*out, ErrorKind::Fail))); | ||
| } |
There was a problem hiding this comment.
This method will return Fail for every possible error, which will make debugging complicated. Instead, this could justify returning a different error kind. Maybe the context feature could help?
There was a problem hiding this comment.
replying to the context suggestion: this may not easy to use here. Maybe a custom error type would be better
There was a problem hiding this comment.
What if a made a custom error type with a subenum for the errors? Something like:
ErrorKind::DtlsCombine(DtlsCombineError)
There was a problem hiding this comment.
The usual method in nom is to create a custom error type, an implement From<ErrorKind> and ParseError. See x509-parser errors for an example.
But, please wait before changing errors, because this will add code, and this can be done in a second step
Hi, |
2 participants
# REQUIRES #47 TO LAND FIRST #This PR builds upon #47. The aim is to provide a correct helper that combines DTLS fragments into a full message. The test case illustrates how the parser + combine helper are expected to be used together.