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

QUIC Conformance Updates: Frame, Packet and Stream State Handling #21135

Closed
wants to merge 48 commits into from

Conversation

hlandau
Copy link
Member

@hlandau hlandau commented Jun 6, 2023

New in this PR:

3d0f879f23 QUIC QSM: Get rid of recv_fin_retired in favour of recv_state
093caab4d4 QUIC QSM: Update API documentation
e1413f237f QUIC CONFORMANCE/APL: Handle FIN/reset retirement correctly
948e3d1bc0 QUIC Send Stream State: Transition to DATA_SENT
d7ee7d2080 QUIC CONFORMANCE: Wire the DATA_SENT state
87c6cecdac QUIC APL: Validate receive stream state
6f2b723a5f QUIC APL: Validate send stream state
681f4b36e0 QUIC CONFORMANCE: Stop handling frames after termination
0f4bd9da9b QUIC Conformance: Frame Handling Tests
34cd1746c2 QUIC CONFORMANCE: RFC 9000 s. 9.6
1b8729baa0 QUIC CONFORMANCE: Validate preferred_addr transport parameter
5d72419526 QUIC CONFORMANCE: RFC 9000 s. 19.16: RETIRE_CONNECTION_ID frames
1047e1494e QUIC CONFORMANCE: RFC 9000 s. 19.15: NEW_CONNECTION_ID frames
9fb58f3b1b QUIC RXDP: Make ACK eliciting definition more resilient and centralised
9f5b13bcf8 QUIC CONFORMANCE: RFC 9000 s. 19.14: STREAMS_BLOCKED Frames
35c5e6ac15 QUIC CONFORMANCE: RFC 9000 s. 19.13: STREAM_DATA_BLOCKED Frames
1b65314407 QUIC CONFORMANCE: RFC 9000 s. 19.7
5c58c030cc RFC 9000 s. 19.8: Enforce maximum stream size
51c5087904 QUIC CONFORMANCE: RFC 9000 s. 17.2.5.1
4aed860616 QUIC CONFORMANCE: RFC 9000 s. 17.2.2: Enforce no initial token from server
6b829c0ffc QUIC CONFORMANCE: Enforce packet header reserved bits
a7b36cf8c6 QUIC WIRE: Allow encoding/decoding of reserved header bits
2491024729 QUIC CONFORMANCE: RFC 9000 s. 13.3: MAX_STREAM_DATA generation
3a8e75e2c0 QUIC CONFORMANCE: RFC 9000 s. 12.5: Application CONNECTION_CLOSE frame masking
16d5cd141f QUIC CONFORMANCE: RFC 9000 s. 12.5: Ensure CFQ can not be used to send disallowed frame types in a given PN space
6671f9178e QUIC CONFORMANCE: RFC 9000 s. 12.3: PN Limit
3e75935c8b QUIC CONFORMANCE: RFC 9000 s. 12.3: PN duplicate suppression
ac8b561245 QUIC ACKM: Clarify the role of is_inflight
acca8026db QUIC CONFORMANCE: Enforce minimal frame type encoding
aae6a4a0ce QUIC CONFORMANCE: Packet handling fixes
a472d8a68b QUIC CONFORMANCE: Handle RESET_STREAM final size correctly
f413aaf2a4 QUIC CONFORMANCE: Validate RESET_STREAM final sizes correctly
f0bb62b882 QUIC: Note that we do not retransmit stream data for retransmitted streams
ea4a0f645c QUIC QSM: Free unneeded stream buffers, calculate RESET_STREAM final size correctly
a21a46d893 QUIC QSM: Model final sizes and handle STOP_SENDING correctly
1d21787268 QUIC CONFORMANCE: RFC 9000 s. 3.3: Stream States — Permitted Frame Types — STREAM
a6066c6966 QUIC QSM/STREAM: Refactor to use RFC stream states

@hlandau hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: refactor The issue/pr requests/implements refactoring tests: present The PR has suitable tests present labels Jun 6, 2023
@hlandau hlandau self-assigned this Jun 6, 2023
@arapov arapov linked an issue Jun 9, 2023 that may be closed by this pull request
@mattcaswell
Copy link
Member

This looks like it needs a rebase

@hlandau
Copy link
Member Author

hlandau commented Jun 29, 2023

Updated.

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

I started to review this but only got as far as the first commit :-)

My comments so far...

include/internal/quic_stream_map.h Outdated Show resolved Hide resolved
ssl/quic/quic_channel.c Outdated Show resolved Hide resolved
/* Stream without send part - caller error. */
return 0;

case QUIC_SSTREAM_STATE_SEND:
Copy link
Member

Choose a reason for hiding this comment

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

Might be simpler just to do (same comment for next function):

if (qs->send_state != QUIC_SSTREAM_STATE_SEND)
    return 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

I could. I chose to do it like this as I think it makes the state-based handling more explicit/easier to understand.

ssl/quic/quic_txp.c Outdated Show resolved Hide resolved
include/internal/quic_stream_map.h Outdated Show resolved Hide resolved
ssl/quic/quic_stream_map.c Outdated Show resolved Hide resolved
ssl/quic/quic_stream_map.c Show resolved Hide resolved
ssl/quic/quic_txp.c Outdated Show resolved Hide resolved
include/internal/quic_types.h Outdated Show resolved Hide resolved
ssl/quic/quic_channel.c Outdated Show resolved Hide resolved
test/quic_multistream_test.c Outdated Show resolved Hide resolved
include/internal/quic_stream_map.h Outdated Show resolved Hide resolved
ssl/quic/quic_stream_map.c Outdated Show resolved Hide resolved
@hlandau
Copy link
Member Author

hlandau commented Jul 3, 2023

Updated.

@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Jul 3, 2023
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Great work! Just a few minor nits

Comment on lines +158 to +161
unsigned int type : 8; /* QUIC_STREAM_INITIATOR_*, QUIC_STREAM_DIR_* */

unsigned int send_state : 8; /* QUIC_SSTREAM_STATE_* */
unsigned int recv_state : 8; /* QUIC_RSTREAM_STATE_* */
Copy link
Member

Choose a reason for hiding this comment

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

Would these be better as uint8_t?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not in the habit of using uint8_t. Use of unsigned char with bitfields is not allowed by strict C89. Since these are aligned on 8-bit boundaries they can get optimised to byte load/stores anyway.

include/internal/quic_stream_map.h Outdated Show resolved Hide resolved
include/internal/quic_stream_map.h Outdated Show resolved Hide resolved
ssl/quic/quic_impl.c Outdated Show resolved Hide resolved
ssl/quic/quic_rx_depack.c Outdated Show resolved Hide resolved
ssl/quic/quic_rx_depack.c Outdated Show resolved Hide resolved
ssl/quic/quic_txp.c Outdated Show resolved Hide resolved
test/quic_multistream_test.c Outdated Show resolved Hide resolved
@hlandau
Copy link
Member Author

hlandau commented Jul 6, 2023

Updated.

MacOS is failing, will look into it.

@t8m
Copy link
Member

t8m commented Jul 12, 2023

There is now a conflict, can you please rebase?

openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
…erver

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21135)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QUIC Conformance Updates — Frame handling
5 participants