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

[core,caps] Allow invalid TS_GENERAL_CAPABILITYSET::protocolVersion #10202

Merged
merged 1 commit into from
May 21, 2024

Conversation

akallabeth
Copy link
Member

some FreeRDP versions did send an invalid value of 0x0000 instead of the required 0x200. Log this kind of violation but continue.

some FreeRDP versions did send an invalid value of 0x0000 instead of the
required 0x200. Log this kind of violation but continue.
@akallabeth akallabeth added this to the next-3.6.0 milestone May 20, 2024
@akallabeth akallabeth requested a review from hardening May 20, 2024 13:13
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11998/

@akallabeth
Copy link
Member Author

@freerdp-bot test

@akallabeth
Copy link
Member Author

@freerdp-bot test again please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11999/

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/12000/

Copy link
Contributor

@hardening hardening left a comment

Choose a reason for hiding this comment

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

Looks ok for me, in rdp_write_general_capability_set we should also ignore the value from the settings and always send TS_CAPS_PROTOCOLVERSION.

@akallabeth
Copy link
Member Author

@freerdp-bot test
@hardening hmm, you mean removing the setting?

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/12001/

@hardening
Copy link
Contributor

hardening commented May 21, 2024

@hardening hmm, you mean removing the setting?

I guess that at end that what we will do. But in the meantime that would prevent behaviors like in proxies where you have front settings that you copy as is for the back connection. And so if an old FreeRDP connects front you will fail to connect back because of the check in the write function. Another possibility is to not store the value in the settings when reading the capability set...

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/12002/

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/12003/

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/12004/

@akallabeth akallabeth merged commit 440fafe into FreeRDP:master May 21, 2024
6 of 7 checks passed
@akallabeth akallabeth deleted the relax-protocol branch May 21, 2024 09:22
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.

None yet

3 participants