Skip to content
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

Always check CRYPTO_LOCK_{read,write}_lock #14238

Closed
wants to merge 1 commit into from

Conversation

@richsalz
Copy link
Contributor

@richsalz richsalz commented Feb 18, 2021

Some functions that lock things are void, so we just return early.

Fixes: #14230

Also marks the lock routines __owur so the error doesn't propagate.

/*
* Can't return something, so best to hope that something will
* fail later. :(
*/

This comment has been minimized.

@kroeckx

kroeckx Feb 25, 2021
Member

Is there a reason you can't make this function return something?

This comment has been minimized.

@richsalz

richsalz Feb 25, 2021
Author Contributor

it's a new_func function used in the crypto "exdata" stuff. Look how "new_func" is used in crypto/ex_data.c Changing it to non-void would be lots of work and subtly breaking things.

crypto/core_namemap.c Outdated Show resolved Hide resolved
crypto/core_namemap.c Outdated Show resolved Hide resolved
crypto/engine/eng_pkey.c Outdated Show resolved Hide resolved
crypto/engine/eng_pkey.c Outdated Show resolved Hide resolved
crypto/evp/keymgmt_lib.c Outdated Show resolved Hide resolved
crypto/ex_data.c Show resolved Hide resolved
@richsalz
Copy link
Contributor Author

@richsalz richsalz commented Feb 25, 2021

rebased to fix conflicts. will wait until you are done with the review @kroeckx . Thanks for the incredibly detailed comments!

crypto/provider_core.c Outdated Show resolved Hide resolved
crypto/rand/rand_lib.c Outdated Show resolved Hide resolved
crypto/x509/by_dir.c Outdated Show resolved Hide resolved
crypto/x509/by_dir.c Outdated Show resolved Hide resolved
ssl/ssl_sess.c Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:check-locks branch from 6feeddd to 3b6b517 Feb 25, 2021
@richsalz
Copy link
Contributor Author

@richsalz richsalz commented Feb 25, 2021

Pushed a fixup commit that address all of @kroeckx 's wonderful detailed feedback. (Not gonna lie: it was a pain going through and responding, but he was pretty much always right so it made this PR better.)

@richsalz richsalz force-pushed the richsalz:check-locks branch from 3b6b517 to abe1f62 Feb 26, 2021
@openssl-machine openssl-machine force-pushed the openssl:master branch from b6b1457 to d2ccfb9 Feb 26, 2021
@richsalz richsalz force-pushed the richsalz:check-locks branch from abe1f62 to 0fc02fb Feb 26, 2021
@mattcaswell mattcaswell added this to the 3.0.0 beta1 milestone Mar 8, 2021
@richsalz richsalz force-pushed the richsalz:check-locks branch from 0fc02fb to 9c2b44a Mar 8, 2021
@richsalz
Copy link
Contributor Author

@richsalz richsalz commented Mar 8, 2021

rebased to fix conflict ("Cert" vs "cert" in a comment so important :) and had to add a couple of additional cases to core_provider. updated fixup commit pushed.

Copy link
Contributor

@paulidale paulidale left a comment

Oopsed it I think :(

crypto/provider_core.c Outdated Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:check-locks branch from 9c2b44a to 3b59059 Mar 8, 2021
crypto/provider_core.c Outdated Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:check-locks branch from 3b59059 to cb9eb13 Mar 9, 2021
@richsalz richsalz requested a review from paulidale Mar 9, 2021
@paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 11, 2021

Ping for review.

@richsalz richsalz mentioned this pull request Mar 11, 2021
1 of 1 task complete
crypto/core_namemap.c Outdated Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:check-locks branch from 82578da to 7995861 Mar 11, 2021
@slontis
Copy link
Contributor

@slontis slontis commented Mar 11, 2021

rebase time also..

@richsalz richsalz force-pushed the richsalz:check-locks branch from 7995861 to 2ed7ec8 Mar 11, 2021
@richsalz
Copy link
Contributor Author

@richsalz richsalz commented Mar 11, 2021

rebased to fix small conflict

@slontis
Copy link
Contributor

@slontis slontis commented Mar 12, 2021

looks like rebase pulled in a new change :) Error..

a.c Outdated Show resolved Hide resolved
Some functions that lock things are void, so we just return early.

Also make ossl_namemap_empty return 0 on error.  Updated the docs, and added
some code to ossl_namemap_stored() to handle the failure, and updated the
tests to allow for failure.

Fixes: #14230
@richsalz
Copy link
Contributor Author

@richsalz richsalz commented Mar 12, 2021

YARS (Yet another rebase and squash) because someone added a locking call and the CI broke.

@richsalz richsalz force-pushed the richsalz:check-locks branch from 362c0f0 to c4114dd Mar 12, 2021
@openssl-machine
Copy link

@openssl-machine openssl-machine commented Mar 14, 2021

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 14, 2021

CI isn't relevant. Merged to master, thanks for the contribution.

@paulidale paulidale closed this Mar 14, 2021
openssl-machine pushed a commit that referenced this pull request Mar 14, 2021
Some functions that lock things are void, so we just return early.

Also make ossl_namemap_empty return 0 on error.  Updated the docs, and added
some code to ossl_namemap_stored() to handle the failure, and updated the
tests to allow for failure.

Fixes: #14230

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #14238)
@richsalz richsalz deleted the richsalz:check-locks branch Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants