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

openssl: use of variables defined in different scopes #16246

Closed
MarcelRaad opened this issue Feb 7, 2025 · 6 comments
Closed

openssl: use of variables defined in different scopes #16246

MarcelRaad opened this issue Feb 7, 2025 · 6 comments
Labels

Comments

@MarcelRaad
Copy link
Member

I did this

With warnings-as-errors enabled, Visual Studio 2022 17.4.21 issues the following errors for the 8.12.0 release:

lib\vtls\openssl.c(1595): error C2220: the following warning is treated as an error
lib\vtls\openssl.c(1595): warning C4701: potentially uninitialized local variable 'ca' used
lib\vtls\openssl.c(1595): warning C4703: potentially uninitialized local pointer variable 'ca' used
lib\vtls\openssl.c(1594): warning C4701: potentially uninitialized local variable 'x509' used
lib\vtls\openssl.c(1594): warning C4703: potentially uninitialized local pointer variable 'x509' used
lib\vtls\openssl.c(1593): warning C4701: potentially uninitialized local variable 'pri' used
lib\vtls\openssl.c(1593): warning C4703: potentially uninitialized local pointer variable 'pri' used

This is a regression from 999cc81#diff-03eaf320162ec20f67fab4e16f494e29735dadfa12425530869d42eea86bd2cc, which added gotos to the fail label defined in a different scope and using local variables from that scope.

I expected the following

No warnings about uninitialized variables.

curl/libcurl version

8.12.0

operating system

Windows

@MarcelRaad MarcelRaad added the TLS label Feb 7, 2025
@MarcelRaad
Copy link
Member Author

Curious that this doesn't show up in CI.

bagder added a commit that referenced this issue Feb 7, 2025
Some of the 'goto fail' situations could happen without having
initialized the local variables referenced in the error code flow.

Reported-by: Marcel Raad
Fixes #16246
@bagder
Copy link
Member

bagder commented Feb 7, 2025

Indeed curious that none of the CI jobs spotted this. Can you check if #16251 does the job?

MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Feb 7, 2025
@MarcelRaad
Copy link
Member Author

Works for me, thanks! I have just created #16252 to see if that shows the same warning for GCC, so we might want to wait for its builds to run before merging your PR.

MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Feb 7, 2025
@MarcelRaad
Copy link
Member Author

It does catch the issue :-)

https://github.com/curl/curl/actions/runs/13209540241/job/36880307494?pr=16252

vtls/openssl.c: In function 'cert_stuff':
vtls/openssl.c:1462:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1462 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1489:23: note: 'ca' declared here
 1489 |       STACK_OF(X509) *ca = NULL;
      |                       ^~
vtls/openssl.c:1462:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1462 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1487:15: note: 'p12' declared here
 1487 |       PKCS12 *p12 = NULL;
      |               ^~~
vtls/openssl.c:1462:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1462 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1486:12: note: 'cert_bio' declared here
 1486 |       BIO *cert_bio = NULL;
      |            ^~~~~~~~
vtls/openssl.c:1[73](https://github.com/curl/curl/actions/runs/13209540241/job/36880307494?pr=16252#step:19:74)9:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1739 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1489:23: note: 'ca' declared here
 1489 |       STACK_OF(X509) *ca = NULL;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1489:23: note: 'ca' declared here
 1489 |       STACK_OF(X509) *ca = NULL;
      |                       ^~
vtls/openssl.c:1739:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1739 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:1487:15: note: 'p12' declared here
 1487 |       PKCS12 *p12 = NULL;
      |               ^~~
vtls/openssl.c:1739:11: error: jump skips variable initialization [-Werror=jump-misses-init]
 1739 |           goto fail;
      |           ^~~~
vtls/openssl.c:1588:1: note: label 'fail' defined here
 1588 | fail:
      | ^~~~
vtls/openssl.c:14[86](https://github.com/curl/curl/actions/runs/13209540241/job/36880307494?pr=16252#step:19:87):12: note: 'cert_bio' declared here
 1486 |       BIO *cert_bio = NULL;
      |            ^~~~~~~~

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Lovely!

@vszakats
Copy link
Member

vszakats commented Feb 8, 2025

Our end of the warnings seems to be working correctly (after double checking
them).

One suspect is OpenSSL configuration. It's kind of hard to tell what options
do vcpkg and AppVeyor use in their OpenSSL builds, let alone figuring out
the build defaults.

This curl codepath needs the OpenSSL UI feature. So I added an #error
there and sure enough, it's not hit either:

https://github.com/curl/curl/actions/runs/13210670321/job/36883398358
https://ci.appveyor.com/project/curlorg/curl/builds/51476216

It looks like vcpkg/AppVeyor OpenSSL binaries do not have UI enabled, which
would make sense because in a headless CI env, a UI popup hangs the job.

This also explains why I bumped into these warnings with iOS and the Carthage
OpenSSL build, which must have left UI enabled.

edit: to give it a CI test, we may force-enable this section and add the necessary
OpenSSL functions as stubs.
UI is enabled in our custom quictls build in GHA/http3-linux.

@bagder bagder closed this as completed in 3f79695 Feb 8, 2025
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Feb 8, 2025
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Feb 16, 2025
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants