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.

Sign up for GitHub

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

Introduce EVP_PKEY_meth_destroy function #4356

Closed
wants to merge 2 commits into from
Closed

Introduce EVP_PKEY_meth_destroy function #4356

wants to merge 2 commits into from

Conversation

InfoHunter
Copy link
Member

@InfoHunter InfoHunter commented Sep 11, 2017 •
edited

To address travis failure in #4337 . This function is used to delete all application added pmeth. Only the last commit is related. Only last two commits are related to this PR. The first commit is loaned from #4337 ...

Checklist
  • documentation is added or updated
  • tests are added or updated

@levitte
Copy link
Member

levitte commented Sep 11, 2017

This doesn't strike me as the right way to do things... or rather, it was fitting for pre-1.1.0 OpenSSLs, where we asked the application authors to do all the libcrypto cleanup explicitely. Starting with 1.1.0, we've made that an internal matter instead.

So I'm thinking that EVP_PKEY_meth_destroy should be made libcrypto internal and be called from OPENSSL_cleanup, probably close to the call of evp_cleanup_int.

@InfoHunter
Copy link
Member Author

If so, EVP_PKEY_meth_destroy could be placed in OPENSSL_cleanup and be made internal. If EVP_PKEY_meth_destroy is not public anymore, should there at least be one other function like EVP_PKEY_meth_delete or similar to delete one pmeth which is added by the application previously? Otherwise there is no way for the application to delete one pmeth from the library-maintained app_pkey_methods except destroying them all by calling OPENSSL_cleanup directly (which is obviously not encouraged...)

@levitte
Copy link
Member

levitte commented Sep 12, 2017

Good point. EVP_PKEY_meth_remove0? And then an internal functions that cleans away the whole application stack.

@InfoHunter
Copy link
Member Author

EVP_PKEY_meth_remove0?

Sure.

@richsalz
Copy link
Contributor

WAIT, what does remove0 even mean? We have set0/get0 which are explainable in terms of refcounting. Remove0 seems strange.

@InfoHunter
Copy link
Member Author

Oops, EVP_PKEY_meth_remove should be enough...

@levitte
Copy link
Member

levitte commented Sep 12, 2017

Well, the function to add a pmeth to the internal application stack is EVP_PKEY_meth_add0, so the idea with remove0 is to match the adder name.

@richsalz
Copy link
Contributor

But we could have an add1 if there was need. The 1 and 0 are about refcounts, which make no sense in remove

@InfoHunter
Copy link
Member Author

I searched the code and seems all functions suffixed with _remove or _delete have no trailing numbers. Although we can describe _remove0 as 'has nothing to do with refcount', but I think _remove might be more proper in this case (this is also another kind of consistency...)

Changes are made in rebased commits. This time the only the last commit is related.

@levitte
Copy link
Member

levitte commented Sep 12, 2017

The 0 and 1 suffixes mostly tell you if the object that's passed will be used as is or "duplicated". Some "duplication" is done by increasing the refcount, when the object has that. If not, the "duplication" is done by copying memory.

(note that the 0 / 1 suffix is a relatively new invention, we have APIs that are older and don't use those suffixes)

But ok, it was only a suggestion, I don't really have a problem with _remove

levitte
levitte requested changes Sep 12, 2017
View reviewed changes
crypto/evp/pmeth_lib.c
@@ -294,6 +296,21 @@ int EVP_PKEY_meth_add0(const EVP_PKEY_METHOD *pmeth)
return 1;
}

void evp_app_cleanup_int(void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps actually add a call to it in OPENSSL_cleanup? ;-)
(you'll have to add a declaration in crypto/include/internal/evp_int.h too)

Also, may I suggest a rename to evp_pkey_cleanup_int?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I've already added this stuff in evp_cleanup_int which will be called by OPENSSL_cleanup and the header file evp_int.h was also updated in the commit.

Also, may I suggest a rename to evp_pkey_cleanup_int

pkey is too wide, I think evp_pkey_meth_cleanup_int would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, now I see the change that adds it... dunno why github didn't show me that earlier...

levitte
levitte requested changes Sep 13, 2017
View reviewed changes
crypto/evp/names.c
@@ -90,6 +90,8 @@ void evp_cleanup_int(void)

EVP_PBE_cleanup();
OBJ_sigid_free();

evp_app_cleanup_int();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I know why I was confused. I expected this call to appear in OPENSSL_cleanup, not here. Frankly, I'm not sure this is the right spot...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All EVP related cleanups should be grouped together and be called once in OPENSSL_cleanup, shouldn't they?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, yeah ok...

@levitte levitte added the approval: review pending This pull request needs review by a committer label Sep 13, 2017
@InfoHunter
Copy link
Member Author

I am wondering if this is finally approved, should I remove the 'helper' commit (ef64d5e) before merging? Thus it can be more easy to merge the related one(s)...or you may have better ideas.

@levitte
Copy link
Member

levitte commented Sep 13, 2017

I think #4337 should be merged first, then it should be easy to merge only the top commit from this one

@richsalz
Copy link
Contributor

Yes, Paul. Wait for 4337 then rebase and 'push -f'

@InfoHunter
Copy link
Member Author

Okay

@InfoHunter InfoHunter mentioned this pull request Sep 13, 2017
Closed
2 tasks
@levitte
Copy link
Member

levitte commented Sep 13, 2017

Ok, time for a rebase here

@mspncp mspncp mentioned this pull request Sep 13, 2017
Closed
1 task
@mspncp
Copy link
Contributor

mspncp commented Sep 13, 2017

Ok, seems like I have wait for this one to be fixed ;-) I am getting the same ci errors in #4328.

paulidale
paulidale reviewed Sep 14, 2017
View reviewed changes
doc/man3/EVP_PKEY_meth_new.pod Outdated
@@ -369,6 +374,9 @@ object or returns NULL if not found.
EVP_PKEY_meth_add0() returns 1 if method is added successfully or 0
if an error occurred.

EVP_PKEY_meth_remove() returns 1 if method is removed successfully or
0 if the method has not been added previously.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see: 0 if an error occurred. here.

I realize the only error possible at the moment is if the item isn't in the stack but that might not be true forever.

@InfoHunter
Support EVP_PKEY_meth_remove and pmeth internal cleanup
6574bae 
1. make app pkey methods cleanup internal
2. add EVP_PKEY_meth_remove

Fixes travis-ci failure in openssl#4337
@InfoHunter
Copy link
Member Author

Rebased and @paulidale 's suggestion is also taken.

@paulidale
Copy link
Contributor

@levitte to reconfirm.

levitte
levitte requested changes Sep 14, 2017
View reviewed changes
doc/man3/EVP_PKEY_meth_new.pod Outdated
@@ -350,6 +352,9 @@ then the built-in objects.

EVP_PKEY_meth_add0() adds B<pmeth> to the user defined stack of methods.

EVP_PKEY_meth_remove() removes an B<EVP_PKEY_METHOD> object added by
EVP_PKEY_meth_new().
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I think you meant EVP_PKEY_meth_add0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that I didn't catch this earlier :-/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, originally I wanted to express 'created by _new and added by _add0'. Now seems just mention _add0 is enough...

@InfoHunter
Fix doc nits
c571fcc 
[to be squashed]
[skip ci]
@InfoHunter
Copy link
Member Author

New commit pushed, please re-review @levitte @paulidale

levitte pushed a commit that referenced this pull request Sep 14, 2017
@InfoHunter
Support EVP_PKEY_meth_remove and pmeth internal cleanup
0822e89 
1. make app pkey methods cleanup internal
2. add EVP_PKEY_meth_remove

Fixes travis-ci failure in #4337

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #4356)
levitte pushed a commit that referenced this pull request Sep 14, 2017
@InfoHunter
Fix doc nits
a8f730d 
[to be squashed]
[skip ci]

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #4356)
@InfoHunter
Copy link
Member Author

This is merged, so now close this PR...

@InfoHunter InfoHunter closed this Sep 14, 2017
@InfoHunter InfoHunter deleted the meth-del branch September 14, 2017 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: review pending This pull request needs review by a committer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@InfoHunter @levitte @richsalz @mspncp @paulidale