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.

Sign up for GitHub

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

Jump to bottom

lost_pn can go negative #2611

Closed
larseggert opened this issue Apr 12, 2019 · 3 comments · Fixed by #2651
Closed

lost_pn can go negative #2611

larseggert opened this issue Apr 12, 2019 · 3 comments · Fixed by #2651
Assignees
@janaiyengar @ianswett
Labels
-recovery

Comments

@larseggert
Copy link
Member

larseggert commented Apr 12, 2019 •
edited

in DetectLostPackets, lost_pn is computed as

lost_pn = largest_acked_packet[pn_space] - kPacketThreshold

However, largest_acked_packet is initialized to zero

largest_acked_packet[pn_space] = 0

and so the subtraction can cause lost_pn to go negative when DetectLostPackets is called before an ACK is received (since it will be updated only in OnAckReceived).

Suggest to initialize largest_acked_packet to infinity and to explicitly check for this case.
@marten-seemann
Copy link
Contributor

I support this proposal, since the way we’re initializing largest_acked_packet is wrong: we’re initializing it to 0, although we decided that 0 is a valid packet number.

ianswett added a commit that referenced this issue Apr 15, 2019
@ianswett
lost_pn can no longer go negative
19ee6d7 
And 0 is a valid packet number, so defaulting largest_acked_packet to 0 is inherently incorrect.

Fixes #2611
ianswett added a commit that referenced this issue Apr 15, 2019
@ianswett
lost_pn can no longer be negative
b84d5cb 
And 0 is a valid packet number, so defaulting largest_acked_packet to 0 is inherently incorrect.

Fixes #2611
This was referenced Apr 15, 2019
Merged
Closed
ianswett added a commit that referenced this issue Apr 19, 2019
@ianswett
lost_pn can no longer be negative (#2621)
82c82d5 
* lost_pn can no longer be negative

And 0 is a valid packet number, so defaulting largest_acked_packet to 0 is inherently incorrect.

Fixes #2611

* Update draft-ietf-quic-recovery.md

Marten's suggestion

* Update draft-ietf-quic-recovery.md

Don't subtract

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md
@ianswett
Copy link
Contributor

Closed by #2621

@ianswett
Copy link
Contributor

I merged #2611 into a random local branch instead of head, so I'm reopening this.

@ianswett ianswett reopened this Apr 24, 2019
ianswett added a commit that referenced this issue Apr 24, 2019
@ianswett
Remove lost_pn and avoid integer underflow
a23c6e6 
Fixed version of #2611 which was inadvertently merged into a local branch.
@ianswett ianswett mentioned this issue Apr 24, 2019
Merged
ianswett added a commit that referenced this issue Apr 29, 2019
@ianswett
Remove lost_pn and avoid integer underflow (#2651)
7f2c268 
* Remove lost_pn and avoid integer underflow

Fixed version of #2611 which was inadvertently merged into a local branch.

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@janaiyengar janaiyengar

@ianswett ianswett

Labels
-recovery
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove lost_pn and avoid integer underflow quicwg/base-drafts
4 participants
@larseggert @marten-seemann @janaiyengar @ianswett