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

Prevent write at NULL when codec configured not to use multiple threads #9712

Merged

Conversation

alega-kas
Copy link

Progressive codec accessed 0 memory address when the following condition was met:

[HKEY_LOCAL_MACHINE\Software\FreeRDP\FreeRDP\RemoteFX]
"UseThreads"=dword:00000000

Inside progressive.c: When RFX_CONTEXT_PRIV::UseThreads was set to 0,
progressive_process_tiles() function wrote at memory address 0.

@freerdp-bot
Copy link

Can one of the admins verify this patch?

libfreerdp/codec/progressive.c Outdated Show resolved Hide resolved
@alega-kas alega-kas force-pushed the fix_codec_progressive_failure branch from 1a6a456 to ec3b7a2 Compare January 9, 2024 06:02
@alega-kas alega-kas changed the base branch from master to stable-2.0 January 9, 2024 06:03
Copy link
Member

@akallabeth akallabeth left a comment

Choose a reason for hiding this comment

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

LGTM

@akallabeth akallabeth added this to the oldstable-2.0-next milestone Jan 9, 2024
@akallabeth
Copy link
Member

@freerdp-bot test

@freerdp-bot
Copy link

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

@alega-kas
Copy link
Author

@akallabeth
Extract from build output https://ci.freerdp.com/job/ios/11176/console :

/Users/freerdp/ci.freerdp.com/workspace/ios/source/winpr/libwinpr/crypto/cipher.c:69:2: error: call to undeclared function 'EVP_CIPHER_CTX_init'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        EVP_CIPHER_CTX_init((EVP_CIPHER_CTX*)ctx);

Does the error mean CI is not configured for PRs into stable-2.0 branch properly?

@akallabeth
Copy link
Member

@alega-kas means we did not yet backport openssl 3 support to stable-2.0

@alega-kas
Copy link
Author

alega-kas commented Jan 9, 2024

@alega-kas means we did not yet backport openssl 3 support to stable-2.0
@akallabeth
Need I do anything else regarding this PR? Is any action expected from my side?

@akallabeth
Copy link
Member

@freerdp-bot test

@akallabeth
Copy link
Member

@alega-kas no, I´m fixing this.

@alega-kas
Copy link
Author

OK, I stop working on this PR.
In case my action is required, please mention me explicitly.

@freerdp-bot
Copy link

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

@akallabeth
Copy link
Member

@alega-kas ok, now it compiles. you need to format your changes though, the format checker fails.

When RFX_CONTEXT_PRIV::UseThreads is set to 0,
progressive_process_tiles() function will not attempt
to write at memory address 0.
@alega-kas alega-kas force-pushed the fix_codec_progressive_failure branch from ec3b7a2 to 55c3bb8 Compare January 9, 2024 09:47
@alega-kas
Copy link
Author

alega-kas commented Jan 9, 2024

@alega-kas ok, now it compiles. you need to format your changes though, the format checker fails.

@akallabeth
I've fixed clang-format claim.

It seems freerdp-bot need to be launched manually.

@akallabeth
Copy link
Member

@freerdp-bot test

@freerdp-bot
Copy link

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

@akallabeth
Copy link
Member

@freerdp-bot test

@freerdp-bot
Copy link

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

@akallabeth akallabeth merged commit 1d929ae into FreeRDP:stable-2.0 Jan 9, 2024
3 checks passed
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