Subject: [quicwg/base-drafts] Fix for #3187 (#3188)

From: Antoine Delignat-Lavaud <[email-masked]>
Date: Mon, 04 Nov 2019 10:21:45 -0800

You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3188

-- Commit Summary --

  * Fix for #3187

-- File Changes --

    M draft-ietf-quic-transport.md (7)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3188.patch
https://github.com/quicwg/base-drafts/pull/3188.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Nov 2019 15:58:19 -0800

martinthomson approved this pull request.

> +   // Note the extra check for overflow when candidate_pn
+   // is near MAX_PACKET_NUMBER

I think that we can use a single comment here for both.
```c
// Extra checks are required to prevent overflow and underflow.
```

> @@ -6057,12 +6057,15 @@ DecodePacketNumber(largest_pn, truncated_pn, pn_nbits):
    // The following code calculates a candidate value and
    // makes sure it's within the packet number window.
    candidate_pn = (expected_pn & ~pn_mask) | truncated_pn
-   if candidate_pn <= expected_pn - pn_hwin:
+   // Note the extra check for overflow when candidate_pn
+   // is near MAX_PACKET_NUMBER
+   if candidate_pn <= expected_pn - pn_hwin and
+      candidate_pn + pn_win < 1 << 62:

If the intent is to avoid overflow, then it might be a good idea to avoid overflow on intermediate values also:

```suggestion
      candidate_pn < (1 << 62) - pn_win:
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188#pullrequestreview-311433258

---

From: Antoine Delignat-Lavaud <[email-masked]>
Date: Tue, 05 Nov 2019 12:39:08 +0000 (UTC)

@ad-l pushed 1 commit.

f9e9cc72f27d80b2d11197ca4a8edb9b28c475c6  Integrate Martin Thomson's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188/files/6f5b524c1ac2ed0892748d01d8037256a1456f32..f9e9cc72f27d80b2d11197ca4a8edb9b28c475c6

---

From: Antoine Delignat-Lavaud <[email-masked]>
Date: Tue, 05 Nov 2019 12:39:45 +0000 (UTC)

ad-l commented on this pull request.

Yes, agreed

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188#discussion_r342540281

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Nov 2019 15:09:48 -0800

Merged #3188 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188#event-2773340798

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Nov 2019 15:09:51 -0800

Thanks!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3188#issuecomment-550065051