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

Add atexit configuration option to control using atexit() in libcrypto at build-time. #23394

Closed
wants to merge 1 commit into from

Conversation

rsbeckerca
Copy link
Contributor

@rsbeckerca rsbeckerca commented Jan 25, 2024

This fixes an issue with a mix of atexit() usage in DLL and statically linked libcrypto that came out in the test suite on NonStop, which has slightly different DLL unload processing semantics compared to Linux. The change allows a build configuration to select whether to register OPENSSL_cleanup() with atexit() or not, so avoid situations where atexit() registration causes SIGSEGV.

INSTALL.md and CHANGES.md have been modified to include and describe this
option.

The no-atexit option has been added to .github/workflows/run-checker-daily.yml.

Fixes: #23135

Signed-of-by: Randall S. Becker randall.becker@nexbridge.ca

Checklist
  • documentation is added or updated
  • tests added

@rsbeckerca rsbeckerca marked this pull request as draft January 25, 2024 22:17
@rsbeckerca rsbeckerca changed the title Add OPENSSL_NO_DLL_ATEXIT_HANDLERS to control whether to use atexit(). [WIP] Add OPENSSL_NO_DLL_ATEXIT_HANDLERS to control whether to use atexit(). Jan 25, 2024
@rsbeckerca rsbeckerca marked this pull request as ready for review January 26, 2024 01:13
@rsbeckerca rsbeckerca changed the title [WIP] Add OPENSSL_NO_DLL_ATEXIT_HANDLERS to control whether to use atexit(). Add OPENSSL_NO_DLL_ATEXIT_HANDLERS to control whether to use atexit(). Jan 26, 2024
@mattcaswell
Copy link
Member

Note that you can already control this at run time via OPENSSL_init_crypto(OPENSSL_INIT_NO_ATEXIT, NULL)

NOTES-NONSTOP.md Outdated Show resolved Hide resolved
crypto/init.c Outdated Show resolved Hide resolved
@rsbeckerca
Copy link
Contributor Author

Note that you can already control this at run time via OPENSSL_init_crypto(OPENSSL_INIT_NO_ATEXIT, NULL)

Runtime is not really an option. There are hundreds of applications deployed using OpenSSL that would all require modification. High-impact changes like this would be problematic. This was briefly commented on in #23135 .

@rsbeckerca rsbeckerca changed the title Add OPENSSL_NO_DLL_ATEXIT_HANDLERS to control whether to use atexit(). Add atexit configuration option to using atexit() in libcrypto at build-time. Jan 26, 2024
@rsbeckerca rsbeckerca changed the title Add atexit configuration option to using atexit() in libcrypto at build-time. Add atexit configuration option to control using atexit() in libcrypto at build-time. Jan 26, 2024
@rsbeckerca
Copy link
Contributor Author

This PR passed all regression tests/checks and is ready for review again.

@@ -420,6 +420,7 @@ my @disablables = (
"asan",
"asm",
"async",
"atexit",
Copy link
Member

Choose a reason for hiding this comment

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

This new option should additionally be documented in the INSTALL.md file since it applies to all platforms. I think it also justifies a CHANGES.md entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be there on the next push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates done. Thank you for your feedback.

@mattcaswell mattcaswell added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member labels Jan 29, 2024
@mattcaswell
Copy link
Member

Probably, it should also have an entry in .github/workflows/run-checker-daily.yml

@rsbeckerca
Copy link
Contributor Author

Probably, it should also have an entry in .github/workflows/run-checker-daily.yml

I have added a no-atexit entry in the list of no-* entries. Hopefully that is sufficient. Next push will have it. Thanks for the feedback.

…ld-time.

This fixes an issue with a mix of atexit() usage in DLL and statically linked
libcrypto that came out in the test suite on NonStop, which has slightly
different DLL unload processing semantics compared to Linux. The change
allows a build configuration to select whether to register OPENSSL_cleanup()
with atexit() or not, so avoid situations where atexit() registration causes
SIGSEGV.

INSTALL.md and CHANGES.md have been modified to include and describe this
option.

The no-atexit option has been added to .github/workflows/run-checker-daily.yml.

Fixes: openssl#23135

Signed-of-by: Randall S. Becker <randall.becker@nexbridge.ca>
@mattcaswell mattcaswell added tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature labels Jan 29, 2024
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer and removed approval: otc review pending This pull request needs review by an OTC member labels Jan 29, 2024
@mattcaswell
Copy link
Member

@nhorman - please reconfirm

@nhorman
Copy link
Contributor

nhorman commented Jan 29, 2024

approval reconfirmed.

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 30, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 31, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@tmshort
Copy link
Contributor

tmshort commented Feb 2, 2024

Merged to master
Thank you for your contribution!

@tmshort tmshort closed this Feb 2, 2024
openssl-machine pushed a commit that referenced this pull request Feb 2, 2024
…ld-time.

This fixes an issue with a mix of atexit() usage in DLL and statically linked
libcrypto that came out in the test suite on NonStop, which has slightly
different DLL unload processing semantics compared to Linux. The change
allows a build configuration to select whether to register OPENSSL_cleanup()
with atexit() or not, so avoid situations where atexit() registration causes
SIGSEGV.

INSTALL.md and CHANGES.md have been modified to include and describe this
option.

The no-atexit option has been added to .github/workflows/run-checker-daily.yml.

Fixes: #23135

Signed-of-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #23394)
Sashan pushed a commit to Sashan/openssl that referenced this pull request Feb 12, 2024
…ld-time.

This fixes an issue with a mix of atexit() usage in DLL and statically linked
libcrypto that came out in the test suite on NonStop, which has slightly
different DLL unload processing semantics compared to Linux. The change
allows a build configuration to select whether to register OPENSSL_cleanup()
with atexit() or not, so avoid situations where atexit() registration causes
SIGSEGV.

INSTALL.md and CHANGES.md have been modified to include and describe this
option.

The no-atexit option has been added to .github/workflows/run-checker-daily.yml.

Fixes: openssl#23135

Signed-of-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#23394)
@hlandau
Copy link
Member

hlandau commented Feb 13, 2024

OTC vote has passed: openssl/technical-policies#89

This can be backported to 3.0, 3.1 and 3.2 (without the NonStop changes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

04-test_encoder_decoder.t broken in master on NonStop x86
6 participants