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

fix assertion in h2_frame_parser for StreamPriority #16158

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nektro
Copy link
Member

@nektro nektro commented Jan 4, 2025

discovered because test/js/node/test/parallel/test-http2-client-set-priority.js crashes in debug mode
did not add to this pr because there are still remaining failures in the file

per https://httpwg.org/specs/rfc7540.html#PRIORITY

Weight: An unsigned 8-bit integer representing a priority weight for the stream (see Section 5.3). Add one to the value to obtain a weight between 1 and 256.

thus .weight is actually supposed to be offset by one

cc @cirospaciari

@robobun
Copy link

robobun commented Jan 4, 2025

Updated 1:06 PM PT - Jan 6th, 2025

@cirospaciari, your commit 09b75b2 is building: #9019

@@ -2660,7 +2660,7 @@ pub const H2FrameParser = struct {

var priority: StreamPriority = .{
.streamIdentifier = stream_identifier.toUInt32(),
.weight = @truncate(stream.weight),
.weight = @truncate(stream.weight - 1),
Copy link
Member

Choose a reason for hiding this comment

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

Good catch

@nektro nektro marked this pull request as ready for review January 4, 2025 19:14
Copy link
Member

@cirospaciari cirospaciari left a comment

Choose a reason for hiding this comment

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

Looks like stream.weight is being set in more places we should add tests for this in specific and check to not being less than zero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants