Skip to content

/ openssl

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

Jump to bottom

Modify ossl_provider_forall_loaded() to avoid locking for the callbacks #14489

Closed
wants to merge 4 commits into from
Closed

Modify ossl_provider_forall_loaded() to avoid locking for the callbacks #14489

wants to merge 4 commits into from

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 10, 2021 •
edited by slontis

This avoids recursive lock issues by taking a copy of the provider list and he callbacks are made without holding the store lock.

Fixes #14251

  • documentation is added or updated
  • tests are added or updated
@paulidale
core: modify ossl_provider_forall_loaded() to avoid locking for the c…
e4ff6e6 
…allbacks

To avoid recursive lock issues, a copy is taken of the provider list and
the callbacks are made without holding the store lock.

Fixes #14251
@paulidale paulidale added branch: master approval: review pending labels Mar 10, 2021
@paulidale paulidale added this to the Sprint Lithium milestone Mar 10, 2021
@paulidale paulidale self-assigned this Mar 10, 2021
@slontis
slontis reviewed Mar 10, 2021
View changes
Copy link
Contributor

@slontis slontis left a comment

update the second commit to not say 'atatement'..
@slontis
Copy link
Contributor

@slontis slontis commented Mar 10, 2021

Commit should also say Fixes #??
@slontis
slontis reviewed Mar 10, 2021
View changes
doc/internal/man3/ossl_provider_new.pod Show resolved Hide resolved
@slontis
slontis reviewed Mar 10, 2021
View changes
crypto/provider_core.c Show resolved Hide resolved
@slontis
slontis reviewed Mar 10, 2021
View changes
crypto/provider_core.c Show resolved Hide resolved
@paulidale paulidale force-pushed the paulidale:forall-loaded branch from c46e5db to 171251c Mar 10, 2021
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 10, 2021

The first commit includes a Fixes line.
@paulidale paulidale linked an issue that may be closed by this pull request Mar 10, 2021
ossl_provider_forall_loaded should not hold a lock while calling the user callback #14251
Closed
@slontis
Copy link
Contributor

@slontis slontis commented Mar 10, 2021

The first commit includes a Fixes line.

Added to the PR comment above also
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 10, 2021

@levitte, any reason this function is forall while all the rest are doall?
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 10, 2021

The suggestion is to rename this ossl_provider_doall_activated()
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 10, 2021

... and renamed.
@mattcaswell
mattcaswell requested changes Mar 10, 2021
View changes
crypto/provider_core.c Show resolved Hide resolved
doc/internal/man3/ossl_provider_new.pod Show resolved Hide resolved
paulidale added 3 commits Mar 10, 2021
@paulidale
doc: describe the return from ossl_provider_forall_loaded()
ce2dfcf 
Also correct an incorrect statement about non-activated providers.
@paulidale
rename ossl_provider_forall_loaded to ossl_provider_doall_activated
bc47fa8
@paulidale
fixup! doc: describe the return from ossl_provider_forall_loaded()
Loading status checks…
0dbb53f
@paulidale paulidale force-pushed the paulidale:forall-loaded branch from d1e8ddb to 0dbb53f Mar 10, 2021
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 10, 2021

Feedback addressed again.
@slontis
slontis approved these changes Mar 10, 2021
View changes
Copy link
Contributor

@slontis slontis left a comment

LGTM..

The concerning thing here is the test failure in what seems like the rsa keygen..
I am sticking this is a loop to see if I can get it to happen again... Although I have not seen this error before and that code has been in for a very long time now..
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 11, 2021

It's a heisen-bug :(
@paulidale paulidale added approval: done and removed approval: review pending labels Mar 11, 2021
@slontis
Copy link
Contributor

@slontis slontis commented Mar 11, 2021

It's a heisen-bug :(

I cant be certain :)

I have been running that line in a loop for an hour with no issues.
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 11, 2021

My guess would be that it was related to the random seed being used. RSA keygen can fail if it takes too long. It looked like long output.
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 11, 2021

Merged to master, thanks for the feedbacks.
@paulidale paulidale closed this Mar 11, 2021
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
@paulidale
core: modify ossl_provider_forall_loaded() to avoid locking for the c…
7bbfbc8 
…allbacks

To avoid recursive lock issues, a copy is taken of the provider list and
the callbacks are made without holding the store lock.

Fixes #14251

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14489)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
@paulidale
doc: describe the return from ossl_provider_forall_loaded()
3c5ce1c 
Also correct an incorrect statement about non-activated providers.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14489)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
@paulidale
rename ossl_provider_forall_loaded to ossl_provider_doall_activated
Loading status checks…
8f08957 
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14489)
@paulidale paulidale deleted the paulidale:forall-loaded branch Mar 11, 2021
@paulidale paulidale added approval: ready to merge and removed approval: done labels Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell

@slontis slontis

Assignees

@paulidale paulidale

Labels
approval: ready to merge branch: master
Projects
None yet
Milestone
Sprint Lithium
Linked issues

Successfully merging this pull request may close these issues.

ossl_provider_forall_loaded should not hold a lock while calling the user callback
3 participants
@paulidale @slontis @mattcaswell