Skip to content

Enforce SETTINGS frame rules#9864

Merged
duke8253 merged 1 commit intoapache:masterfrom
duke8253:master-h3_settings_frame
Jun 21, 2023
Merged

Enforce SETTINGS frame rules#9864
duke8253 merged 1 commit intoapache:masterfrom
duke8253:master-h3_settings_frame

Conversation

@duke8253
Copy link
Contributor

this resolves #9428

@duke8253 duke8253 added this to the 10.0.0 milestone Jun 16, 2023
@duke8253 duke8253 requested a review from maskit June 16, 2023 15:34
@duke8253 duke8253 self-assigned this Jun 16, 2023
@duke8253 duke8253 force-pushed the master-h3_settings_frame branch from dbebd24 to 0008e99 Compare June 16, 2023 15:58

protected:
Http3Error(){};
Http3Error() : app_error_code(Http3ErrorCode::H3_NO_ERROR){};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default error code is not 0 anymore due to #9793. I can make a separate PR for this if needed.


// Dispatch
Http3FrameType type = frame->type();
if (stream_type == Http3StreamType::CONTROL) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this into a private function? This block will be very long eventually. For example, we also need to check this:

If a DATA frame is received on a control stream, the recipient MUST respond with a connection error of type H3_FRAME_UNEXPECTED.

I was going to make ProtocolEnforcer that inherits FrameHandler and handle all frame types (as you can see we can have multiple frame handlers for a frame type). With the enforcer, a dispatcher can focus on dispatching. The Single Responsibility Principle.

@duke8253 duke8253 force-pushed the master-h3_settings_frame branch from 0008e99 to 2dff907 Compare June 20, 2023 20:47
Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for updating the code.

@maskit
Copy link
Member

maskit commented Jun 20, 2023

[approve ci autest]

@duke8253 duke8253 merged commit 39dac9f into apache:master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Prohibit a second SETTINGS frame

2 participants