Skip to content
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

implement the 3x amplification limit #2536

Merged
merged 4 commits into from
May 27, 2020
Merged

Conversation

marten-seemann
Copy link
Member

Fixes #1862.

func (p *packetPacker) packCoalescedPacket(buffer *packetBuffer, maxPacketSize protocol.ByteCount) (*coalescedPacket, error) {
maxPacketSize = utils.MinByteCount(maxPacketSize, p.maxPacketSize)
if p.perspective == protocol.PerspectiveClient {
maxPacketSize = protocol.MinInitialPacketSize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as client we're always ignoring the parameter? Should we make that clearer, e.g. maxPacketSizeForServer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for coalesced packet sent by the client. This doesn't apply for normal packets.

internal/ackhandler/sent_packet_handler.go Outdated Show resolved Hide resolved
internal/ackhandler/sent_packet_handler.go Outdated Show resolved Hide resolved
@@ -1427,7 +1428,7 @@ func (s *session) sendPacket() (bool, error) {

if !s.handshakeConfirmed {
now := time.Now()
packet, err := s.packer.PackCoalescedPacket(protocol.MaxByteCount)
packet, err := s.packer.PackCoalescedPacket(s.sentPacketHandler.AmplificationWindow())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a risk for busy-looping here, if we exit the send loop when it's time to send an ACK? Or is this the same as if we're congestion limited, and we'll deal with that gracefully?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled the same way as being congestion limited.

integrationtests/self/self_suite_test.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann added this to the v0.16 milestone May 23, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #2536 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2536      +/-   ##
==========================================
+ Coverage   86.19%   86.22%   +0.03%     
==========================================
  Files         122      122              
  Lines        9583     9603      +20     
==========================================
+ Hits         8260     8280      +20     
  Misses        985      985              
  Partials      338      338              
Impacted Files Coverage Δ
internal/ackhandler/received_packet_handler.go 72.88% <100.00%> (+0.47%) ⬆️
internal/ackhandler/sent_packet_handler.go 71.98% <100.00%> (+1.06%) ⬆️
packet_packer.go 84.95% <100.00%> (+0.07%) ⬆️
session.go 75.37% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd5ecee...e33f7d0. Read the comment docs.

@marten-seemann marten-seemann merged commit 85c19fb into master May 27, 2020
@marten-seemann marten-seemann deleted the amplification-protection branch May 27, 2020 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement the amplification protection
3 participants