Repository navigation
Conversation
A NEW_CONNECTION_ID frame that took the number of active connection IDs past our advertised active_connection_id_limit was logged and dropped. RFC 9000 section 5.1.1 requires closing the connection with CONNECTION_ID_LIMIT_ERROR, which had no raise site. - Close with CONNECTION_ID_LIMIT_ERROR when the limit is exceeded - Add a test covering at-limit, repeated and over-limit frames
|
Thanks for working on this. Your known limitation is indeed correct. |
|
@deadcaf3 would you be able to work on the known limitation? I'm worried that by merging this PR we may start closing connections just due to packet loss or reordering. |
A connection ID that we retire ourselves is removed from the remote list, so a late retransmission of its NEW_CONNECTION_ID frame was taken for a new connection ID. It was added back when there was room, and with a full list it closed the connection with CONNECTION_ID_LIMIT_ERROR. RFC 9000 section 19.15 says receiving the same frame more than once must not be a connection error. - Remember the sequence numbers a connection ID list has held - Ignore a NEW_CONNECTION_ID frame whose sequence number was held before - Add tests for a repeated frame after local retirement
fixed in Edited and updated the PR body |
|
|
||
| // Sequence numbers this list has held, remembered after they are retired so that a repeated | ||
| // NEW_CONNECTION_ID frame for a retired connection ID is not taken for a new one. | ||
| private var heldSequenceNumbers = RangeSet<UInt64>() |
There was a problem hiding this comment.
Is there a way to avoid the cost of this approach for localCIDs?
There was a problem hiding this comment.
agreed, fixed in a87c427
| } | ||
|
|
||
| private mutating func recordHeld(sequenceNumber: UInt64) { | ||
| heldSequenceNumbers.insert(contentsOf: sequenceNumber..<sequenceNumber + 1) |
There was a problem hiding this comment.
We could be more explicit about the fact that they are just 62 bit only (with a comment). and avoid the overflow check:
| heldSequenceNumbers.insert(contentsOf: sequenceNumber..<sequenceNumber + 1) | |
| heldSequenceNumbers.insert(contentsOf: sequenceNumber..<sequenceNumber &+ 1) |
There was a problem hiding this comment.
nice to make it explicit, avoiding crashes, fixed in a87c427
…e list Only the list of the peer's connection IDs receives NEW_CONNECTION_ID frames, so it is the only list that needs to remember the sequence numbers it has held. The local list now records nothing. Sequence numbers are variable-length integers, at most 2^62 - 1, so the increment uses a wrapping add instead of an overflow-checked one.
|
When heldSequenceNumbers is {0, 2} because seq=1 was lost and when we receive seq=3 retirePriorTo=3, heldSequenceNumbers will be {0, 2, 3}. The the retransmission of seq=1 will not close the gap on heldSequenceNumbers because we return here: frame.sequence < retiredRemoteCIDSequenceNumberThreshold |
A NEW_CONNECTION_ID frame whose sequence number is below the Retire Prior To threshold is retired on arrival and never reaches the list, so a frame that was lost and later retransmitted left a permanent gap in the held sequence numbers. Fill in everything below the threshold when it rises, so a gap can only be a frame above it that is lost or in flight. The cap on gaps stays as a backstop against a peer that ignores the limit.
…imit is checked RFC 9000 section 5.1.1 lets a peer exceed active_connection_id_limit when the same NEW_CONNECTION_ID frame retires the excess. Cover that path so the limit check cannot be moved ahead of retirement.
Edit: you're correct, and |
2 participants
RFC 9000 section 5.1.1:
processNewConnectionIDFramelogged and dropped a connection ID that went over the limit, and.connectionIDLimitErrorhad no raise site.Change
CONNECTION_ID_LIMIT_ERRORwhen a NEW_CONNECTION_ID frame would take the number of active remote connection IDs past the limit we advertised, and returnfalselike the other close sites in the function.retireOutboundCID(forPathGoingAway:),sendRetireConnectionIDFrame) is removed from the list, so a late retransmission of its frame was taken for a new connection ID: added back when there was room, and counted against the limit when there was not. Section 19.15 says receiving the same frame more than once must not be a connection error.QUICConnectionIDListkeeps the sequence numbers it has held in aRangeSet, which is a single range when every frame arrives before a later Retire Prior To passes it. A frame that arrives after that is retired without being held, so it leaves a gap. Gaps are capped at twice the limit, oldest closed first.Testing
testNewConnectionIDOverLimitClosesConnection: reaching the limit is accepted, a repeated frame is ignored, and one connection ID over the limit closes the connection.testRepeatedNewConnectionIDForLocallyRetiredCIDIsIgnored: a repeated frame for a connection ID we retired is not added back, and does not close the connection once the peer has replaced it.testHasHeldAfterRetireandtestHasHeldClosesOldestGapcover the list.swift testpasses locally on macOS (1283 test cases, 0 failures). Both connection tests fail without their change.