Never destroy module-wide singletons via static teardown

[rikard-soderstrom]

SoftHSM::instance, MutexFactory::instance, SecureMemoryRegistry::instance, and OSSLCryptoFactory::instance/BotanCryptoFactory::instance were each a class-level static std::unique_ptr (or std::auto_ptr pre-C++11). Their destructors ran as part of the C++ runtime's ordinary static-object teardown at process exit - an indeterminate time relative to unrelated code's own cleanup.

Concretely: a PKCS#11 caller (e.g. an OpenSSL provider) that still holds open sessions at process exit calls C_CloseSession() on each before calling C_Finalize(), per the PKCS#11 spec. But SoftHSM::instance's own destructor could fire first, via ordinary static teardown, independently of and unordered with respect to that caller's explicit cleanup sequence. Whichever teardown path lost the race operated on an already-destroyed (or transparently reconstructed-blank, since SoftHSM::i() rebuilds a fresh instance on next access) singleton, and its stale session-close calls then dereferenced a NULL member on it - a real, reproducible SIGSEGV when a pkcs11-provider-based caller runs openssl req against a PKCS#11 key and exits normally.

This is the same category of bug already fixed once for OSSLCryptoFactory's own OpenSSL ENGINE cleanup (issue #548, commit c2cc065), just never applied to these singletons' own top-level lifecycle - and the gap resurfaces one level down too: OSSLCryptoFactory can be lazily constructed for the first time from deep inside SoftHSM::C_Finalize()'s own teardown chain, touching ENGINE_by_id() after OpenSSL's own OPENSSL_cleanup() has already run.

Each singleton's std::unique_ptr/std::auto_ptr is now heap-allocated once and deliberately never freed, so nothing destroys the T it owns except that class's own explicit reset()/C_Finalize() path - never ordinary static teardown. The smart pointer itself is retained as the owning type (matching the "Static and Global Variables" guidance in the Google C++ Style Guide, and the same "leaky singleton" idiom used by e.g. Drake's never_destroyed), so reset()'s exception-safety and ownership semantics for the owned object are unchanged.

Summary by CodeRabbit

• Bug Fixes

  • Improved initialization and cleanup reliability for core cryptographic, memory-management, synchronization, and SoftHSM components.
  • Preserved consistent behavior across modern and legacy C++ build configurations.
  • Maintained compatibility with both OpenSSL and Botan cryptographic backends.

• Tests

  • Updated related test coverage and initialization paths to reflect the improved component lifecycle handling.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 19b7f16d-944e-445c-9dd7-2f6b6e2dff3e

📥 Commits

Reviewing files that changed from the base of the PR and between f12916e and 208164e.

📒 Files selected for processing (17)

• src/bin/util/softhsm2-util.cpp
• src/lib/SoftHSM.cpp
• src/lib/SoftHSM.h
• src/lib/common/MutexFactory.cpp
• src/lib/common/MutexFactory.h
• src/lib/crypto/BotanCryptoFactory.cpp
• src/lib/crypto/BotanCryptoFactory.h
• src/lib/crypto/OSSLCryptoFactory.cpp
• src/lib/crypto/OSSLCryptoFactory.h
• src/lib/crypto/test/cryptotest.cpp
• src/lib/data_mgr/SecureMemoryRegistry.cpp
• src/lib/data_mgr/SecureMemoryRegistry.h
• src/lib/data_mgr/test/datamgrtest.cpp
• src/lib/handle_mgr/test/handlemgrtest.cpp
• src/lib/object_store/test/objstoretest.cpp
• src/lib/session_mgr/test/sessionmgrtest.cpp
• src/lib/slot_mgr/test/slotmgrtest.cpp

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

Singleton storage changed from direct smart-pointer objects to pointers to dynamically allocated smart-pointer wrappers. Production accessors, reset paths, utility code, and tests support both C++11 and legacy builds.

Changes

Singleton holder migration

Layer / File(s)	Summary
Singleton holder declarations and definitions src/lib/SoftHSM.h, src/lib/SoftHSM.cpp, src/lib/common/MutexFactory.h, src/lib/crypto/*Factory.h, src/lib/data_mgr/SecureMemoryRegistry.h	Singleton members and definitions now store pointers to dynamically allocated unique_ptr or auto_ptr objects. Comments document the wrapper lifetime.
Singleton access and lifecycle updates src/lib/SoftHSM.cpp, src/lib/common/MutexFactory.cpp, src/lib/crypto/*Factory.cpp, src/lib/data_mgr/SecureMemoryRegistry.cpp	Accessors and reset paths now call smart-pointer methods through the pointer wrapper. Fork detection and recreation use the updated indirection.
Utility and test initialization sites src/bin/util/softhsm2-util.cpp, src/lib/*/test/*.cpp	Static singleton definitions now allocate empty smart-pointer wrappers with new in C++11 and legacy branches. OpenSSL and Botan selection remains unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 20816

The change moves singleton destruction to explicit lifecycle paths, preventing unsafe process-exit teardown ordering without changing public behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: bukka

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 17 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preventing module-wide singleton destruction during static teardown.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}