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

Always call OPENSSL_cleanup prior to exit #22544

Closed
wants to merge 1 commit into from

Conversation

nhorman
Copy link
Contributor

@nhorman nhorman commented Oct 27, 2023

If an engine is loaded during the course of operations, and if that engine is written in C++, the data in that library will be deleted using an atexit handler prior to the execution of openssl's atexit handler, causing memory corruption/segfaults/unpredictable behavior. The only way to avoid that is to release any reference we have to that data prior to exit, which means calling OPENSSL_cleanup prior to exit. This patch enforces that behavior for all openssl utilities

Fixes #22508

If an engine is loaded during the course of operations, and if that
engine is written in C++, the data in that library will be deleted using
an atexit handler prior to the execution of openssl's atexit handler,
causing memory corruption/segfaults/unpredictable behavior.  The only
way to avoid that is to release any reference we have to that data prior
to exit, which means calling OPENSSL_cleanup prior to exit.  This patch
enforces that behavior for all openssl utilities

Fixes openssl#22508
@beldmit
Copy link
Member

beldmit commented Oct 27, 2023

Shouldn't we also alter the apps_startup function to call the OPENSSL_init_ssl function with OPENSSL_INIT_NO_ATEXIT flag?

@nhorman
Copy link
Contributor Author

nhorman commented Oct 27, 2023

hmm, possibly, but I think we're safe one way or the other here. If we call OPENSSL_cleanup immediately prior to exit, then the 2nd call to OPENSSL_cleanup from the atexit handler should reduce to a noop (as the stopped variable will be set to 1).

We certainly could call OPENSSL_init_ssl in or prior to apps_startup, with OPENSSL_INIT_NO_ATEXIT, and that would arguably be more correct, but I don't think it will affect the behavior.

@bernd-edlinger
Copy link
Member

note: there are lots of exit(1); all over in the apps folder.
@levitte most of them should probably be EXIT(1); in order for vms to know that is an error-exit?

@paulidale
Copy link
Contributor

EXIT(EXIT_FAILURE) rather.

@t8m
Copy link
Member

t8m commented Oct 30, 2023

IMO this is not a fix but a workaround and it should not be applied as it just papers over the problem for the openssl application. But any third party application would have to be changed the same way and that is unacceptable.

@mattcaswell
Copy link
Member

This is a significant change in direction. Ever since 1.1.0 we have always said that applications do not need to explicitly call OPENSSL_cleanup and recommended that approach to our users. Developers regularly look at the source code of our own apps as the basis for how to write things, so I don't think we should be making this change without a clear idea what our final solution to the "atexit" problem is going to be. As I mention in this comment there are problems with simply mandating OPENSSL_cleanup everywhere.

@paulidale
Copy link
Contributor

@mattcaswell agreed. Let's figure out a solution properly rather than rushing into something.

@nhorman nhorman marked this pull request as ready for review November 1, 2023 17:03
@nhorman nhorman closed this Nov 8, 2023
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

6 participants