Subject: [quicwg/base-drafts] use the FRAME_ENCONDING_ERROR for errors in frame encoding (#3042)

From: Marten Seemann <[email-masked]>
Date: Mon, 16 Sep 2019 19:34:42 -0700

Fixes #3027.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * use the FRAME_ENCONDING_ERROR for errors in frame encoding

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3042.patch
https://github.com/quicwg/base-drafts/pull/3042.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/3042

---

From: Martin Thomson <[email-masked]>
Date: Mon, 16 Sep 2019 20:17:38 -0700

martinthomson approved this pull request.

LGTM, with maybe a note on the flow control one.

> @@ -5061,7 +5060,7 @@ The first byte in the stream has an offset of 0.  The largest offset delivered
 on a stream - the sum of the offset and data length - cannot exceed 2^62-1, as
 it is not possible to provide flow control credit for that data.  Receipt of a
 frame that exceeds this limit will be treated as a connection error of type
-FLOW_CONTROL_ERROR.
+FRAME_ENCODING_ERROR.

This creates an extra check that some implementations might not bother to implement.  Treating this as a flow control error is also fine.  I note that this doesn't use a MUST either...

-- 
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/3042#pullrequestreview-288999898

---

From: Marten Seemann <[email-masked]>
Date: Mon, 16 Sep 2019 23:55:50 -0700

@marten-seemann pushed 1 commit.

c56dd27788254e1cc6574a3e50a77f16b1fccabd  also allow FLOW_CONTROL_ERRORs for stream overflows

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3042/files/62278ed1a7d4fcac38f88f669339282128ddeb30..c56dd27788254e1cc6574a3e50a77f16b1fccabd

---

From: Marten Seemann <[email-masked]>
Date: Mon, 16 Sep 2019 23:56:14 -0700

marten-seemann commented on this pull request.

Resolved by requiring one of those two.

-- 
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/3042#discussion_r325006667

---

From: ianswett <[email-masked]>
Date: Tue, 17 Sep 2019 09:45:16 -0700

ianswett commented on this pull request.

> @@ -5060,8 +5059,8 @@ the offset of the next byte that would be sent.
 The first byte in the stream has an offset of 0.  The largest offset delivered
 on a stream - the sum of the offset and data length - cannot exceed 2^62-1, as
 it is not possible to provide flow control credit for that data.  Receipt of a
-frame that exceeds this limit will be treated as a connection error of type
-FLOW_CONTROL_ERROR.
+frame that exceeds this limit MUST be treated as a connection error of type
+FRAME_ENCODING_ERROR or FLOW_CONTROL_ERROR.

I'd be inclined towards only recommending FLOW_CONTROL_ERROR here.

-- 
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/3042#pullrequestreview-289402001

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 17 Sep 2019 13:36:07 -0700

janaiyengar commented on this pull request.

I agree with @ianswett that I would prefer FLOW_CONTROL_ERROR since it is a bit more specific. Otherwise looks fine to me.

-- 
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/3042#pullrequestreview-289531000

---

From: Martin Thomson <[email-masked]>
Date: Tue, 17 Sep 2019 18:32:07 -0700

martinthomson commented on this pull request.

I think that permitting either is in line with principles.  I mean, sure, this can be flow control, but if someone has code that validates frames and that code all directs into a FRAME_ENCODING_ERROR code path, that's fine.  Checking that `x+y>=2^62` might be redundant, but I don't want to say that that code can't run that check.

-- 
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/3042#discussion_r325449290

---

From: ianswett <[email-masked]>
Date: Tue, 17 Sep 2019 18:40:24 -0700

I don't think options are helpful here, so I'd prefer the more specific error.

-- 
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/3042#discussion_r325450606

---

From: MikkelFJ <[email-masked]>
Date: Tue, 17 Sep 2019 22:27:11 -0700

mikkelfj commented on this pull request.

If there is a flow control limit < 2^62 then presumably you would check against that and not a theoretical max limits. Thus having two different errors would needlessly add an extra check.

-- 
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/3042#discussion_r325487224

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 17 Sep 2019 22:48:10 -0700

kazuho commented on this pull request.

Note that there is a similar issue with MAX_STREAMS and STREAMS_BLOCKED. They communicate the cumulative count of streams of particular type using varint. It is possible to express numbers up to 2<sup>62</sup> however it is always an error to indicate a value above 2<sup>60</sup>.

[Section 4.5](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#rfc.section.4.5) currently states that STREAM_LIMIT_ERROR is to be used for communicating such error (for MAX_STREAMS, there is no mention of STREAMS_BLOCKED).

-- 
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/3042#discussion_r325491130

---

From: Martin Thomson <[email-masked]>
Date: Wed, 18 Sep 2019 00:17:50 -0700

That was done before this proposed principle.  Do you think that we should be aiming to have more specific error codes where possible here?

-- 
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/3042#discussion_r325516339

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 18 Sep 2019 00:25:56 -0700

My point is that if we are going to permit the use of FRAME_ENCODING_ERROR when the STREAM frame exceeds the 2<sup>62</sup> boundary, then we should also permit the use of FRAME_ENCODING_ERROR for MAX_STREAMS frame carrying a count exceeding 2<sup>60</sup>.

My weak preference is to not do either of the two, as we'd be losing a clear signal. IMO, this discussion is about the error code to be used when integrity check for certain frames fail. We can argue that integrity check is not part of frame decoding. Then, it would be natural to use the more specific error codes.

-- 
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/3042#discussion_r325519338