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

Fix an ENGINE leak in asn1_item_digest_with_libctx #12560

Closed
wants to merge 1 commit into from

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 30, 2020

Commit 6725682 introduced a call to ENGINE_get_digest_engine() into
the function asn1_item_digest_with_libctx() to determine whether there
is an ENGINE registered to handle the specified digest. However that
function increases the ref count on the returned ENGINE object, so it
must be freed.

Fixes #12558

[extended tests]

Note there are other unrelated extended test failures besides this one. Therefore I expect the extended tests to still fail even after this PR.

Commit 6725682 introduced a call to ENGINE_get_digest_engine() into
the function asn1_item_digest_with_libctx() to determine whether there
is an ENGINE registered to handle the specified digest. However that
function increases the ref count on the returned ENGINE object, so it
must be freed.

Fixes #12558

[extended tests]
Copy link
Contributor

@paulidale paulidale left a comment

The Travis errors are the external tests.

The GOST engine is failing to build properly and isn't the fault of the PR.
The boringssl tests are also failing, it doesn't look like it is the fault of this PR but I'm less sure.

@romen
romen approved these changes Jul 30, 2020
Copy link
Member

@romen romen left a comment

LGTM

I checked and this does indeed solve #12558 for me without adding extra failures.

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Jul 31, 2020

The boringssl tests are also failing, it doesn't look like it is the fault of this PR but I'm less sure.

That was already failing and so is not the fault of this PR.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jul 31, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Aug 1, 2020

Merged to master.

@paulidale paulidale closed this Aug 1, 2020
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
Commit 6725682 introduced a call to ENGINE_get_digest_engine() into
the function asn1_item_digest_with_libctx() to determine whether there
is an ENGINE registered to handle the specified digest. However that
function increases the ref count on the returned ENGINE object, so it
must be freed.

Fixes #12558

[extended tests]

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #12560)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Commit 6725682 introduced a call to ENGINE_get_digest_engine() into
the function asn1_item_digest_with_libctx() to determine whether there
is an ENGINE registered to handle the specified digest. However that
function increases the ref count on the returned ENGINE object, so it
must be freed.

Fixes openssl#12558

[extended tests]

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from openssl#12560)
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.

4 participants