Skip to content

Fixed multiple issues reported by Claude Opus 4.6 - #686

Open
frankmorgner wants to merge 5 commits into
masterfrom
fixes
Open

frankmorgner wants to merge 5 commits into
masterfrom
fixes

Conversation

@frankmorgner

@frankmorgner frankmorgner commented Sep 10, 2026

Copy link
Copy Markdown
Member

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.

@frankmorgner
frankmorgner force-pushed the fixes branch 3 times, most recently from 5124459 to e5f24fb Compare September 11, 2026 09:27
- fixes race in EC method init
- fixes legacy EC ex_data reference leak
@olszomal

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Great minds think alike.

@frankmorgner

Copy link
Copy Markdown
Member Author

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?

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.

3 participants