Fixed multiple issues reported by Claude Opus 4.6

[frankmorgner]

Pull Request Type

• Bug fix
• New feature
• Code style / formatting / renaming
• Refactoring (no functional or API changes)
• Build / CI related changes
• Documentation
• Other (please describe):

Related Issue

Issue number: N/A

Current Behavior

New Behavior

Scope of Changes

Testing

• Existing tests
• New tests added
• Manual testing

Additional Notes

License Declaration

• I hereby agree to license my contribution under the project's license.

[olszomal]

Most of the changes look good to me (6741168, bef8342, ed18a62, and 4ebb2a9).

The EC method initialization change in 6f9d3c5 still appears to have race conditions and should be addressed before merging.

It would also be helpful to complete the PR description, since this PR contains several independent fixes and the Current Behavior, New Behavior, Scope of Changes, and Testing sections are currently empty.

[mtrojnar]

Reviewed at 4ebb2a9 against master (fd366be). One blocking finding in the new EC initialization locking; the other changes look reasonable.

The initialization mutex is itself initialized unsafely [High]

src/p11_ec.c:735-740 (also 795-800 and 827-832)

ec_init_lock_initialized is checked and set without synchronization. Two concurrent first calls can therefore initialize the same mutex:

Thread A: observes ec_init_lock_initialized == 0; pauses
Thread B: initializes ec_init_lock, sets the flag, locks it
Thread A: initializes ec_init_lock again while B owns it

Reinitializing an initialized pthread mutex is undefined behavior, potentially causing hangs or defeating mutual exclusion.

I reproduced duplicate initialization attempts with two threads calling the public PKCS11_get_ec_key_method(), using an interposer to delay pthread_mutex_init() and detect repeated initialization of the same address:

Base: duplicate mutex initialization attempts: 0
PR:   duplicate mutex initialization attempts: 1

The probe suppressed the second actual initialization to avoid undefined behavior; this confirms the invalid attempt, not a reproduced crash. The base already has method-initialization races, but this mutex-lifetime bug is newly introduced.

Suggestion: initialize the lock through a platform-compatible once mechanism, then acquire it before every method-pointer check. The existing outer unlocked check also allows callers to receive the method after allocation but before callbacks are installed; adding this mutex does not fix that pre-existing
publication race. Apply the fix to all three getters and preserve the supported Windows/OpenSSL compatibility paths.

Behavioural changes worth mentioning

The PR includes concurrency changes, not just refactoring. Its description should explain the intended initialization guarantees and include a concurrent-first-use regression test.

[mtrojnar]

The EC method initialization change in 6f9d3c5 still appears to have race conditions and should be addressed before merging.

Great minds think alike.

[frankmorgner]

Indeed I was fiddeling around with the race conditions. Other modifications of global data were flagged as problematic as well, but the naive approach of a fix led to deadlocks in CI.

As I am unfamiliar with the overall codebase, I suggest to remove the threading "fixes" from this PR and let you decide on how to proceed with the possible race conditions. Would that be acceptable?

[mtrojnar]

Sure. Use git rebase -i to drop commit 6f9d3c5 and git push --force-with-lease to push the other commits. I'll correct the ASN1_STRING_length() macro.

Next time, consider committing separate issues as separate commits, which makes this task mach easier.

[mtrojnar]

Indeed I was fiddeling around with the race conditions. Other modifications of global data were flagged as problematic as well, but the naive approach of a fix led to deadlocks in CI.

A race condition in a one-time initialization is a highly unlikely and impractical corner case. It is a perfect example of the kind of issue found by AI that nobody ever observes in real life. Fixing such issues only makes sense when the fix itself is trivial. Otherwise, we trade a questionable benefit for bloated and less maintainable code.