Introduce log.file config option to log to file #837
Conversation
📝 WalkthroughWalkthroughAdds file-based logging: new config key Changes
Sequence Diagram(s)sequenceDiagram
participant SoftHSM as SoftHSM (init/finalize)
participant Config as Configuration
participant LogMod as log module
participant Sink as Sink (file/syslog)
SoftHSM->>Config: getString("log.file","")
Config-->>SoftHSM: path
SoftHSM->>LogMod: setLogFile(path)
alt path non-empty and open succeeds
LogMod->>Sink: open file handle
LogMod-->>SoftHSM: success
else open failed or empty path
LogMod-->>Sink: use syslog (no file)
LogMod-->>SoftHSM: fallback/warn
end
SoftHSM->>LogMod: softHSMLog(...)
LogMod->>Sink: write entry (file or syslog)
SoftHSM->>LogMod: C_Finalize -> closeLogFile()
LogMod->>Sink: close file handle (if open)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
577-603: Add closeLogFile() calls before early return paths in C_Initialize to prevent resource leaks.If
setLogFile()is called at the start ofC_Initialize(line 577) but initialization fails later (e.g., backend selection at line 586 or object store validation at line 603),C_Finalizewill never be invoked becauseisInitialisedremains false (only set at line 620). SinceC_Finalizeskips execution whenisInitialised == false, the log file handle andlogMutexallocated bysetLogFile()will remain open for the process lifetime.Proposed fix
// Configure log file (empty string = use syslog) if (!setLogFile(Configuration::i()->getString("log.file", ""))) { WARNING_MSG("Could not open log file, using syslog"); } // Configure object store storage backend used by all tokens. if (!ObjectStoreToken::selectBackend(Configuration::i()->getString("objectstore.backend", DEFAULT_OBJECTSTORE_BACKEND))) { ERROR_MSG("Could not set the storage backend"); + closeLogFile(); return CKR_GENERAL_ERROR; } sessionObjectStore = new SessionObjectStore(); // Load the object store objectStore = new ObjectStore(Configuration::i()->getString("directories.tokendir", DEFAULT_TOKENDIR), Configuration::i()->getInt("objectstore.umask", DEFAULT_UMASK)); if (!objectStore->isValid()) { WARNING_MSG("Could not load the object store"); delete objectStore; objectStore = NULL; delete sessionObjectStore; sessionObjectStore = NULL; + closeLogFile(); return CKR_GENERAL_ERROR; }
🤖 Fix all issues with AI agents
In @src/lib/common/Configuration.cpp:
- Line 51: The new "log.file" config needs path validation and safe file
creation: in setLogFile (log.cpp) resolve the provided path to an absolute
canonical path (realpath or equivalent), verify it resides inside the allowed
logging directory(s) using the same policy used by the ObjectStore constructor
for directories.tokendir, and reject/return an error if the resolved path
escapes those directories (preventing ../../ traversal). When creating the file,
open it with safe flags (create with explicit mode) and apply the configured
umask/permissions like objectstore.umask does (or chmod right after creation) to
ensure restrictive file permissions; update setLogFile to log detailed errors on
validation/open failures.
In @src/lib/common/log.cpp:
- Around line 77-88: The setLogFile function has a TOCTOU race reading logFile
and a race creating logMutex; update it to initialize the mutex in a thread-safe
way (e.g., replace the lazy raw pointer pattern with a
std::unique_ptr<std::mutex> or similar initialized via std::call_once or static
local initialization) and always acquire that mutex before reading or writing
logFile (so setLogFile and closeLogFile synchronize on the same mutex);
specifically change the logMutex initialization (currently using
MutexFactory::i()->getMutex()) to a one-time/thread-safe init and remove the
unchecked early read of logFile without holding the lock so checks and updates
to logFile happen only while holding the mutex.
- Around line 197-205: Race condition: the code reads the global logFile without
synchronization. Fix by taking logMutex before inspecting logFile: either lock
logMutex, copy logFile to a local FILE* and then (a) call writeLogToFile while
still holding the lock or (b) change/write a helper that accepts the FILE* and
performs the write outside the lock after copying, so closeLogFile cannot set
logFile = nullptr between the check and the write; ensure syslog(path for the
else branch) remains unchanged and reference the symbols logFile, logMutex,
writeLogToFile, and closeLogFile when making the change.
🧹 Nitpick comments (1)
src/lib/common/log.cpp (1)
145-162: Missing null check for mutex before locking.
writeLogToFileassumeslogMutexis non-null when called. While the current flow throughsoftHSMLogcheckslogFile != nullptr(which implies the mutex was created insetLogFile), a defensive null check would make this function safer against future refactoring.The Cppcheck warning about uninitialized
timeinfoat line 154 is a false positive—localtime_spopulates the struct as an output parameter.🛡️ Optional defensive check
static void writeLogToFile(const int loglevel, const char* prependText, const char* msgText) { + if (logMutex == nullptr) return; + MutexLocker lock(logMutex);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/SoftHSM.cppsrc/lib/common/Configuration.cppsrc/lib/common/log.cppsrc/lib/common/log.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/SoftHSM.cpp
🧬 Code graph analysis (2)
src/lib/common/log.h (1)
src/lib/common/log.cpp (4)
setLogFile(77-111)setLogFile(77-77)closeLogFile(113-131)closeLogFile(113-113)
src/lib/SoftHSM.cpp (1)
src/lib/common/log.cpp (4)
setLogFile(77-111)setLogFile(77-77)closeLogFile(113-131)closeLogFile(113-113)
🪛 Cppcheck (2.19.0)
src/lib/common/log.cpp
[error] 154-154: Uninitialized variable
(uninitvar)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (4)
src/lib/SoftHSM.cpp (1)
646-655: Good: log file is closed during C_Finalize.
This ensures file handles/mutexes from file logging don’t persist after PKCS#11 finalization.src/lib/common/log.h (1)
82-83: LGTM!The function declarations are consistent with the existing
setLogLevelpattern and align correctly with the implementations inlog.cpp.src/lib/common/log.cpp (2)
113-131: LGTM!The function correctly closes the file under lock before recycling the mutex. The implementation properly handles the case where neither the mutex nor the file was ever created.
133-143: LGTM!Clean helper function with proper default handling for unexpected log levels.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/lib/common/log.cpp:
- Around line 197-199: The null-check of logFile is done without
synchronization, causing a TOCTOU when calling writeLogToFile; instead, remove
the unsynchronized check and make writeLogToFile acquire logMutex and validate
logFile (and return early if nullptr) before calling fprintf, and ensure
closeLogFile also holds logMutex while fclose and setting logFile = nullptr so
all accesses to logFile are protected by the same mutex; update callers (the
code that currently checks if (logFile != nullptr) before calling
writeLogToFile) to always call writeLogToFile (or re-check after the call) so
logging occurs only when writeLogToFile confirms logFile is valid under the
lock.
🧹 Nitpick comments (2)
src/lib/common/log.cpp (2)
84-87: Lazy mutex initialization is not thread-safe.If two threads call
setLogFileconcurrently whenlogMutexisnullptr, both may attempt to create a mutex, leading to a race condition. While this is unlikely given thatsetLogFileis typically called during single-threaded initialization (fromC_Initialize), it's a latent concurrency risk.Consider using
std::call_onceor double-checked locking with memory barriers for thread-safe lazy initialization.
159-160: Consider error handling for file I/O operations.The
fprintfandfflushcalls don't check for errors. While failures are rare, they can occur (e.g., disk full, I/O errors). For production robustness, consider logging a fallback message to syslog if file writes fail.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/SoftHSM.cppsrc/lib/common/Configuration.cppsrc/lib/common/log.cppsrc/lib/common/log.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/common/log.h
- src/lib/common/Configuration.cpp
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/SoftHSM.cpp
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (2)
src/lib/common/log.cpp (4)
setLogFile(76-110)setLogFile(76-76)closeLogFile(112-130)closeLogFile(112-112)src/lib/common/Configuration.cpp (2)
i(59-67)i(59-59)
🪛 Cppcheck (2.19.0)
src/lib/common/log.cpp
[error] 153-153: Uninitialized variable
(uninitvar)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 3.0
🔇 Additional comments (4)
src/lib/SoftHSM.cpp (1)
646-648: Good: explicit log file close duringC_Finalize.This makes the log-file lifecycle deterministic and prevents leaking a file handle across init/finalize cycles.
src/lib/common/log.cpp (3)
132-142: LGTM!The level-to-string mapping is straightforward and handles all expected log levels with a sensible default.
152-156: Static analyzer false positive.The Cppcheck warning about an uninitialized variable at line 153 is a false positive. Both
localtime_s(Windows) andlocaltime_r(POSIX) initialize thetimeinfostruct as an output parameter.
125-129: Mutex recycling creates race condition if logging occurs during finalization.Between releasing the lock at line 123 and recycling the mutex at lines 127–128, another thread could attempt to call
writeLogToFileorsetLogFile, which would try to use a recycled/invalidated mutex handle. While the code setsisInitialised = falseonly aftercloseLogFile()completes (line 649), the PKCS#11 protocol should guarantee that no other threads are actively logging duringC_Finalizeshutdown. This pattern is safe only if the finalization protocol ensures all logging operations have ceased before this function is called.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/common/log.h (1)
82-83: Consider adding documentation comments for the new API.While the declarations are correct, adding brief documentation would clarify:
- Thread safety expectations (should these be called only during single-threaded init/finalize?)
- Return value semantics for
setLogFile(true = success, false = file open failed but fallback to syslog)- Expected calling context (e.g., "Call from C_Initialize/C_Finalize only")
This would help prevent misuse, especially given the thread-safety issues flagged in the implementation.
📝 Example documentation
/* Function definitions */ bool setLogLevel(const std::string &loglevel); + +/* Configure file-based logging. Must be called during initialization before + * any PKCS#11 operations. Returns false if file cannot be opened (falls back + * to syslog). Pass empty string to clear file logging and use syslog only. */ bool setLogFile(const std::string &logFilePath); + +/* Close log file and release resources. Must be called during finalization + * after all PKCS#11 operations have completed. */ void closeLogFile(); + void softHSMLog(const int loglevel, const char* functionName, const char* fileName, const int lineNo, const char* format, ...);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/SoftHSM.cppsrc/lib/common/Configuration.cppsrc/lib/common/log.cppsrc/lib/common/log.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/SoftHSM.cpp
- src/lib/common/Configuration.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
🧬 Code graph analysis (1)
src/lib/common/log.h (1)
src/lib/common/log.cpp (4)
setLogFile(76-110)setLogFile(76-76)closeLogFile(112-125)closeLogFile(112-112)
🪛 Cppcheck (2.19.0)
src/lib/common/log.cpp
[error] 148-148: Uninitialized variable
(uninitvar)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
🔇 Additional comments (6)
src/lib/common/log.cpp (6)
41-47: LGTM: Appropriate includes and static initialization.The additions of
time.hfor timestamps andMutexFactory.hfor thread safety are appropriate. Static variables are correctly initialized tonullptrfor lazy initialization.
127-137: LGTM: Clean level-to-string conversion.The function correctly maps syslog levels to readable strings with appropriate fallback.
139-156: LGTM: Properly synchronized file writing with timestamp.The function correctly uses
MutexLockerto protect file access (line 141) and handles cross-platform time conversion appropriately.Regarding the static analysis hint: Cppcheck's "uninitialized variable" warning for line 148 is a false positive.
localtime_son Windows initializes thestruct tmparameter—it's not left uninitialized.
76-110: Critical: Unsynchronized access to shared logFile pointer.The function modifies
logFile(lines 92, 102) without holdinglogMutex, creating a race condition with concurrent reads insoftHSMLog(line 192). The comment on line 101 claims "does not need locking" but this is unsafe:
During initialization: Even if
setLogFileis only called from a single thread during init,softHSMLogcould be called by other threads immediately afterlogFileis assigned but before the function returns.During finalization:
closeLogFilecould null outlogFilewhile worker threads are still logging, leading to potential crashes or use-after-free.Impact: Can cause crashes, use-after-free, or inconsistent logging behavior in multi-threaded environments.
🔒 Recommended synchronization approach
Option 1: Protect all accesses to
logFilewith the mutex:bool setLogFile(const std::string &logFilePath) { // Quick return without creating mutex for default configuration if (logFilePath.empty() && logFile == nullptr) { return true; } if (logMutex == nullptr) { // Create mutex for later access logMutex = MutexFactory::i()->getMutex(); } + MutexLocker lock(logMutex); + if (logFile != nullptr) { fclose(logFile); logFile = nullptr; } if (logFilePath.empty()) { return true; } - // This function needs to be called in init so it does not need locking logFile = fopen(logFilePath.c_str(), "a"); if (logFile == nullptr) { syslog(LOG_ERR, "Failed to open log file: %s, using syslog only", logFilePath.c_str()); return false; } return true; }You'll also need to protect the read in
softHSMLog(see separate comment on lines 191-199).⛔ Skipped due to learnings
Learnt from: bukka Repo: softhsm/SoftHSMv2 PR: 837 File: src/lib/common/log.cpp:192-194 Timestamp: 2026-01-10T15:07:10.701Z Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
187-204: Critical: Unsynchronized read of logFile pointer.Line 192 reads
logFilewithout synchronization, racing with modifications insetLogFile/closeLogFile. This can cause:
- Use-after-free: Thread A reads
logFileas non-null, then thread B closes it viacloseLogFile, then thread A callswriteLogToFilewith a dangling pointer.- Torn reads: On some architectures, pointer reads may not be atomic.
- Null dereference: If
logFileis nulled between the check and the call towriteLogToFile.🔒 Recommended fix
Protect the read with the mutex:
const char* msgText = &logMessage[0]; std::string prependStr = prepend.str(); const char* prependText = prependStr.c_str(); // Log to file if configured, otherwise use syslog - if (logFile != nullptr) + if (logMutex != nullptr) { - writeLogToFile(loglevel, prependText, msgText); + MutexLocker lock(logMutex); + if (logFile != nullptr) + { + writeLogToFile(loglevel, prependText, msgText); + } + else + { + syslog(loglevel, "%s%s", prependText, msgText); + } } else { syslog(loglevel, "%s%s", prependText, msgText); }Note:
writeLogToFilealready locks the mutex internally, so this creates nested locking. You may want to refactorwriteLogToFileto have a_lockedvariant that assumes the mutex is already held.Alternatively, if the mutex already exists by the time logging occurs, you could simplify to always check
logMutex != nullptrand hold it during the file check.⛔ Skipped due to learnings
Learnt from: bukka Repo: softhsm/SoftHSMv2 PR: 837 File: src/lib/common/log.cpp:192-194 Timestamp: 2026-01-10T15:07:10.701Z Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
112-125: Critical: Unsynchronized cleanup risks use-after-free.The function closes
logFile(line 116) and recycleslogMutex(line 122) without synchronization, creating race conditions:
File handle race: Another thread in
softHSMLog→writeLogToFilecould be writing tologFilewhile this function closes it, causing write-to-closed-file errors or crashes.Mutex lifecycle race: Recycling
logMutexwhile another thread holds it (inwriteLogToFile) can lead to undefined behavior.Impact: Can cause crashes, corrupted log output, or mutex violations during shutdown.
🔒 Recommended fix with proper synchronization
void closeLogFile() { + // Acquire mutex before cleanup to ensure no active writers + if (logMutex != nullptr) + { + MutexLocker lock(logMutex); + - if (logFile != nullptr) - { - fclose(logFile); - logFile = nullptr; - } + if (logFile != nullptr) + { + fclose(logFile); + logFile = nullptr; + } + } + // Lock released here, safe to recycle mutex if (logMutex != nullptr) { MutexFactory::i()->recycleMutex(logMutex); logMutex = nullptr; } }Note: This assumes no new logging calls will occur after
closeLogFileis invoked. Consider documenting thatC_Finalizemust ensure all PKCS#11 operations have completed before callingcloseLogFile.⛔ Skipped due to learnings
Learnt from: bukka Repo: softhsm/SoftHSMv2 PR: 837 File: src/lib/common/log.cpp:192-194 Timestamp: 2026-01-10T15:07:10.701Z Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
It adds milli seconds and pid
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/common/log.cpp (3)
41-54: GloballogFile/logMutexstate: ensure invariants are explicit (and defensively upheld).
writeLogToFile()assumeslogMutex != nullptrwheneverlogFile != nullptr; that’s true given currentsetLogFile()flow, but it’s fragile if future code sets/clearslogFileelsewhere. Consider asserting or guarding (fallback to syslog) to avoid UB on accidental misuse.
82-116:setLogFile(): if this ever supports reconfiguration, avoid dropping the old file on open failure.Right now you
fclose(logFile)before attempting to open the new path; iffopen()fails, you lose the previous file handle and silently fall back to syslog. If this function is only called during init, it’s fine; if not, consider “open-new-first then swap” under the mutex. Based on learnings, this API is intended forC_Initializeonly—worth documenting inlog.h(or as a function comment) to prevent accidental runtime use.Proposed refactor (open-new-first, then swap)
bool setLogFile(const std::string &logFilePath) { @@ - if (logFile != nullptr) - { - fclose(logFile); - logFile = nullptr; - } - if (logFilePath.empty()) { + if (logFile != nullptr) + { + fclose(logFile); + logFile = nullptr; + } return true; } // This function needs to be called in init so it does not need locking - logFile = fopen(logFilePath.c_str(), "a"); - if (logFile == nullptr) + FILE* newFile = fopen(logFilePath.c_str(), "a"); + if (newFile == nullptr) { syslog(LOG_ERR, "Failed to open log file: %s, using syslog only", logFilePath.c_str()); return false; } + + if (logFile != nullptr) + { + fclose(logFile); + } + logFile = newFile; return true; }
200-217:softHSMLog(): unlockedlogFilecheck is OK under the current contract, but not if runtime toggling is intended.
if (logFile != nullptr) writeLogToFile(...)does a TOCTOU read oflogFilewithout holdinglogMutex. This is fine ifsetLogFile()/closeLogFile()are truly limited toC_Initialize/C_Finalize(per learnings), but would be racy if you ever allow runtime enable/disable. If runtime toggling is desired, either:
- take
logMutexinsoftHSMLog()around thelogFiledecision, or- make
logFilean atomic pointer and keep open/close/write sequencing consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/common/log.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/common/log.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/common/log.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/common/log.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/common/log.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/common/log.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with Botan
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: macOS (openssl)
🔇 Additional comments (1)
src/lib/common/log.cpp (1)
118-131:closeLogFile(): race safety relies on PKCS#11 init/finalize threading guarantees—please keep it documented.Given the intended usage (only during
C_Finalize) this is OK, but thefclose(logFile)+ recyclinglogMutexwould be unsafe if called concurrently withsoftHSMLog(). I’d explicitly document thatcloseLogFile()must not run concurrently with any logging (and that callingC_Finalizeconcurrently is undefined). Based on learnings, that’s the intended contract.
| static void writeLogToFile(const int loglevel, const char* prependText, const char* msgText) | ||
| { | ||
| MutexLocker lock(logMutex); | ||
|
|
||
| #ifdef _WIN32 | ||
| SYSTEMTIME st; | ||
| GetLocalTime(&st); | ||
| fprintf(logFile, "%04d-%02d-%02d %02d:%02d:%02d.%03d [%d] %s: %s%s\n", | ||
| st.wYear, st.wMonth, st.wDay, st.wHour, st.wMinute, st.wSecond, st.wMilliseconds, | ||
| _getpid(), getLevelString(loglevel), prependText, msgText); | ||
| #else | ||
| struct timespec ts; | ||
| clock_gettime(CLOCK_REALTIME, &ts); | ||
| struct tm timeinfo; | ||
| localtime_r(&ts.tv_sec, &timeinfo); | ||
|
|
||
| char basetime[32]; | ||
| strftime(basetime, sizeof(basetime), "%Y-%m-%d %H:%M:%S", &timeinfo); | ||
| fprintf(logFile, "%s.%03ld [%d] %s: %s%s\n", | ||
| basetime, ts.tv_nsec / 1000000, | ||
| (int)getpid(), getLevelString(loglevel), prependText, msgText); | ||
| #endif | ||
|
|
||
| fflush(logFile); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the structure and understand the logging setup
find . -name "log.cpp" -o -name "log.h" | head -20Repository: softhsm/SoftHSMv2
Length of output: 108
🏁 Script executed:
# Check the full log.cpp file to understand initialization
wc -l src/lib/common/log.cppRepository: softhsm/SoftHSMv2
Length of output: 87
🏁 Script executed:
# Look for logFile and logMutex declarations
rg -n "logFile|logMutex" src/lib/common/log.cpp -B 2 -A 2Repository: softhsm/SoftHSMv2
Length of output: 2165
🏁 Script executed:
# Check how these are initialized
rg -n "logFile\s*=" src/lib/common/log.cpp | head -20Repository: softhsm/SoftHSMv2
Length of output: 272
🏁 Script executed:
# Look for the complete log initialization function
rg -n "void.*[Ll]og" src/lib/common/log.cpp | grep -E "(init|Init|setup|Setup)"Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Check if there are other calls to writeLogToFile
rg -n "writeLogToFile" src/lib/common/log.cppRepository: softhsm/SoftHSMv2
Length of output: 211
🏁 Script executed:
# Check what platforms are supported - look at build configs
find . -name "*.ac" -o -name "CMakeLists.txt" -o -name "configure" | grep -E "^\./[^/]+$|CMakeLists|configure\.ac" | head -10Repository: softhsm/SoftHSMv2
Length of output: 321
🏁 Script executed:
# Check for clock_gettime usage and portability handling
rg -n "clock_gettime|CLOCK_REALTIME" src/Repository: softhsm/SoftHSMv2
Length of output: 124
🏁 Script executed:
# Look for platform-specific defines and portability considerations
rg -n "WIN32|_WIN32|HAVE_CLOCK|sun|solaris|aix|hpux" src/lib/common/log.cppRepository: softhsm/SoftHSMv2
Length of output: 95
🏁 Script executed:
# Check the main header to understand platform support declarations
head -50 src/lib/common/log.hRepository: softhsm/SoftHSMv2
Length of output: 2166
🏁 Script executed:
# Check configure.ac for platform support and clock_gettime checks
cat configure.ac | head -100Repository: softhsm/SoftHSMv2
Length of output: 2688
🏁 Script executed:
# Check for specific clock_gettime checks in configure
rg -n "clock_gettime|HAVE_CLOCK" configure.acRepository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Check CMakeLists.txt for platform detection
cat CMakeLists.txt | head -80Repository: softhsm/SoftHSMv2
Length of output: 3231
🏁 Script executed:
# Look for config.h template or any portability headers
find . -name "config.h*" -type fRepository: softhsm/SoftHSMv2
Length of output: 80
🏁 Script executed:
# Check what syslog calls are used elsewhere
rg -n "syslog" src/lib/common/log.cpp | head -20Repository: softhsm/SoftHSMv2
Length of output: 287
🏁 Script executed:
# Search more thoroughly in configure.ac for clock or time checks
cat configure.ac | grep -i "clock\|time\|posix" -C 2Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Search for AC_CHECK_FUNCS in configure.ac
rg -n "AC_CHECK_FUNCS" configure.acRepository: softhsm/SoftHSMv2
Length of output: 93
🏁 Script executed:
# Get full context around configure.ac to see all checks
cat configure.ac | tail -100Repository: softhsm/SoftHSMv2
Length of output: 2226
🏁 Script executed:
# Check if there are any ifdefs or checks for clock_gettime in the codebase
rg -n "HAVE_CLOCK|HAVE_POSIX|__POSIX__|POSIX" src/Repository: softhsm/SoftHSMv2
Length of output: 1483
🏁 Script executed:
# Verify the exact context of how writeLogToFile is called
sed -n '200,215p' src/lib/common/log.cppRepository: softhsm/SoftHSMv2
Length of output: 458
🏁 Script executed:
# Check README or documentation for supported platforms
find . -name "README*" -o -name "INSTALL*" | head -10Repository: softhsm/SoftHSMv2
Length of output: 94
🏁 Script executed:
# Look for any platform-specific issues or CI configs
find . -name ".github" -o -name ".gitlab-ci*" -o -name ".travis*" -o -name "appveyor*" 2>/dev/null | head -20Repository: softhsm/SoftHSMv2
Length of output: 84
🏁 Script executed:
# Check if there's any documentation about supported platforms
cat README.md 2>/dev/null | head -50Repository: softhsm/SoftHSMv2
Length of output: 2281
🏁 Script executed:
# Check if clock_gettime is conditionally compiled elsewhere or if POSIX is assumed
rg -n "localtime_r|strftime|struct tm|timespec" src/lib/common/ -B 2 -A 2Repository: softhsm/SoftHSMv2
Length of output: 713
🏁 Script executed:
# Final check: verify the exact linking and compilation context
cat src/lib/common/CMakeLists.txt | grep -A 20 "add_library\|target_link"Repository: softhsm/SoftHSMv2
Length of output: 930
🏁 Script executed:
# Verify if any platform-specific handling exists for clock functions
rg -n "rt\|clock\|time" src/lib/common/CMakeLists.txtRepository: softhsm/SoftHSMv2
Length of output: 43
Consider adding null-guards and verify clock_gettime build support.
Two low-cost hardening points:
writeLogToFile()is only called with a guard check (if (logFile != nullptr)at line 205), making an internal null-check redundant. However, a defensive guard could protect against future refactoring. Note: the suggestedsyslog()fallback already exists at the higher call level (line 211).clock_gettime(CLOCK_REALTIME, ...)(line 157) is POSIX.1-2001 standard and used unconditionally on non-Windows targets. The build system (configure.ac) lacks an explicitAC_CHECK_FUNCS([clock_gettime])check. Confirm this function is available on all non-Windows platforms your project targets, or add a build-time portability check.
1 participant
This adds new
log.fileconfig option that allows logging to selected file rather than to syslog. This is more useful for debugging and allows separation of SoftHSMv2 logs (syslog currently always uses the application ident so it's not easy to differentiate it from the application logs).Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.