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

WebSocket: add masking bytes also while enabling MASKING_FRAME flag #1426

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

HaseenaSainul
Copy link
Contributor

No description provided.

}

dataFrame[result++] = (_setFlags & MASKING_FRAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's bring this line back to where it was above. We need to set it unconditionally (see next comment), must for now be set to 0 when no masking used, and it will be easier when we later on (I created a Jira issue for it) need to handle the case that these frames can have application data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


dataFrame[result++] = (_setFlags & MASKING_FRAME);
// Now it seems only control message, hence append with masking keys
uint8_t maskKey[4];
Copy link
Contributor

Choose a reason for hiding this comment

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

The following lines may only be executed when the MASKING_FRAME flag is set (so add an if(_setFlags & MASKING_FRAME) != 0) around it.

Also line 135 (in the new version) needs to be changed
(this one: if (((_controlStatus & (REQUEST_CLOSE | REQUEST_PING | REQUEST_PONG)) != 0) && ((result + 1) < maxSendSize))

the "result + 1" was always wrong I think and needs to be result + 2 but it needs to be result + 6 when MASKING_FRAME flag is set to make sure there is enough room for it (so easiest I think is probably to use ((_setFlags & MASKING_FRAME) == 0) ? : ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@HaseenaSainul HaseenaSainul force-pushed the development/websocket-masking branch from e5b6789 to 6f5b8cd Compare October 11, 2023 03:55
@@ -138,7 +132,7 @@ namespace Web {
result += usedSize;
}

if (((_controlStatus & (REQUEST_CLOSE | REQUEST_PING | REQUEST_PONG)) != 0) && ((result + 1) < maxSendSize)) {
if (((_controlStatus & (REQUEST_CLOSE | REQUEST_PING | REQUEST_PONG)) != 0) && (result + ((_setFlags & MASKING_FRAME) ? 6 : 2) < maxSendSize)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

small remark but to make it more inline with our normal coding could you please make it ((_setFlags & MASKING_FRAME) != 0) we do not advocate the use of implicit conversion to boolean

@HaseenaSainul HaseenaSainul force-pushed the development/websocket-masking branch from 6f5b8cd to dc9ae03 Compare October 11, 2023 07:02
@HaseenaSainul HaseenaSainul merged commit 51da54b into master Oct 11, 2023
@HaseenaSainul HaseenaSainul deleted the development/websocket-masking branch October 11, 2023 07:31
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.

2 participants