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(crypto): mac_context_dtor should free MAC #9119

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

qzhuyan
Copy link
Contributor

@qzhuyan qzhuyan commented Nov 27, 2024

In the doc of EVP_MAC_fetch(), https://docs.openssl.org/3.0/man3/EVP_MAC

"The returned value must eventually be freed with EVP_MAC_free(3)."

This also ensures the same behaviour as in EVP_Q_mac which is used by NIF: crypto:mac/4.

note, without this fix there is no memory leak as openssl3 returns the same addr of 'mac' (for the same MAC type), the side effect is the refcnt keeps bumping but I think it still good to have this fix also consider the case where the libcrypto is used by other NIF.

update: 2nd thought, I think it may have the risk of refcnt overflow.
update: yes. it causes refcnt overflow and triggers double free.

Copy link
Contributor

github-actions bot commented Nov 27, 2024

CT Test Results

  2 files   14 suites   5m 48s ⏱️
188 tests 174 ✅  14 💤 0 ❌
470 runs  331 ✅ 139 💤 0 ❌

Results for commit f2acfa0.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

qzhuyan added a commit to qzhuyan/emqx that referenced this pull request Nov 30, 2024
prefer to use crypto:pbkdf2_hmac to avoid refcnt overflow
in OTP26:

erlang/otp#9119
@jhogberg jhogberg added the team:VM Assigned to OTP team VM label Dec 2, 2024
@qzhuyan
Copy link
Contributor Author

qzhuyan commented Dec 2, 2024

also have a drity but handy tool to reproduce the issue.
https://github.com/qzhuyan/lli/blob/main/src/lli.erl#L14

@sverker
Copy link
Contributor

sverker commented Dec 3, 2024

I pushed a commit to your branch where EVP_MAC_fetch is called once when crypto module is loaded and EVP_MAC_free called when it's unloaded.

@qzhuyan
Copy link
Contributor Author

qzhuyan commented Dec 4, 2024

@sverker LGTM.

Feel free to drop my branch if you want a clean PR.

@sverker sverker added the testing currently being tested, tag is used by OTP internal CI label Dec 4, 2024
@sverker
Copy link
Contributor

sverker commented Dec 4, 2024

@qzhuyan I suggest you squash the two commits into one and set yourself as Co-authored-by: in the commit message. Then force push the branch to your github repo.

@qzhuyan qzhuyan force-pushed the fix/william/openssl-mac-refcnt branch from 9d3352a to fceb55c Compare December 4, 2024 14:40
@qzhuyan
Copy link
Contributor Author

qzhuyan commented Feb 3, 2025

hi, is there anything needed from my side?

@qzhuyan qzhuyan force-pushed the fix/william/openssl-mac-refcnt branch from fceb55c to 7b3675f Compare February 4, 2025 09:09
@qzhuyan
Copy link
Contributor Author

qzhuyan commented Feb 4, 2025

My apologies, I just found I used the wrong author's name.
corrected now.

@sverker
Copy link
Contributor

sverker commented Feb 11, 2025

@qzhuyan I forgot about this. It just missed the code freeze for OTP 28.0-rc1. My bad. I will merge it later for inclusion in 28.0-rc2.

In the doc of EVP_MAC_fetch(), https://docs.openssl.org/3.0/man3/EVP_MAC
"The returned value must eventually be freed with EVP_MAC_free(3)."

Co-authored-by: Sverker Eriksson <[email protected]>
Co-authored-by: William Yang <[email protected]>
@sverker sverker removed the testing currently being tested, tag is used by OTP internal CI label Feb 18, 2025
@sverker sverker force-pushed the fix/william/openssl-mac-refcnt branch from 7b3675f to f2acfa0 Compare February 18, 2025 15:01
@sverker sverker added the testing currently being tested, tag is used by OTP internal CI label Feb 18, 2025
@sverker sverker self-requested a review February 18, 2025 15:03
@sverker sverker merged commit be3ca1a into erlang:master Feb 19, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement fix team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants