Add Reassembly support for Swift IPv6#51
Conversation
| var foundFragment = false | ||
|
|
||
| let result = Deserializer.deserialize(&frame, claim: false) { read throws(DeserializationError) in | ||
| try read.skip(4) |
There was a problem hiding this comment.
Shouldn't the first 4 bytes still be validated?
There was a problem hiding this comment.
This would be redundant because the version / flow label was already validated in processInboundFrames, that is why its skipped here when the frame is parsed from the reassembledState.
| try read.uint16NetworkByteOrder(&payloadLength) | ||
| try read.uint8(&firstProto) | ||
| try read.skip(1) | ||
| try read.skip(32) // src and dst addresses |
There was a problem hiding this comment.
Same question: is this being validated anywhere else?
There was a problem hiding this comment.
Same here, the src and dest addresses were validated in processInboundFrames.
| } | ||
| } | ||
| if !forceFlush { | ||
| if reassemblyState == nil { |
There was a problem hiding this comment.
You could revert this and do if let reassemblyState = reassemblyState { and then you avoid reassemblyState?.
There was a problem hiding this comment.
This would not work because reassemblyState is a non-copyable struct and setting it locally would cause ownership issues.
There was a problem hiding this comment.
You can do it with this sort of trick:
switch reassemblyState {
case .some(let state):
// do stuff with state which is now non nil reassemblyState
case .none:
// nil reassemblyState
}
Or you can even do if .case(let state) = reassemblyState tricks.
There was a problem hiding this comment.
This trick does work for setting a property such as .reassemblyID, but not when creating a new IPv6ReassemblyState object. You would run into a partially consuming self issue in that case I believe and would have to devise a strategy for assigning the entire instance to itself again, i.e., .ipv6(instance).
This would also incur extra cycles we don't need to expend, leaving this as-is.
rpaulo
left a comment
rpaulo
left a comment
There was a problem hiding this comment.
Approved pending minor comments.
| return | ||
| } | ||
| guard newValue < 2 ^ 6 else { | ||
| guard newValue < 64 else { |
There was a problem hiding this comment.
Why this change? It's the same value
There was a problem hiding this comment.
This is an interesting case; when running the test testIPEcho we would receive errors for DSCP values less than 50 in IPv6 cases. This line seemed to be doing bitwise XOR instead of calculating 2 to the power of 6, which is what I think was meant here. Reference on bitwise XOR.
There was a problem hiding this comment.
Ohhh good call then. Thanks for fixing.
3 participants
This change adds reassembly support for inbound IPv6 fragments.
This change only supports inbound fragmentation and reassembly. Outbound fragmentation will come in a later change.
This change takes a similar approach to IPv4 reassembly support, if no fragments are detected in
processInboundFramesit will take the fast path and do nothing with reassembly.