Fix memory leak in EdDSA PrintableString handling#858
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReworks Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/crypto/OSSLUtil.cpp (1)
211-229: Consider usingelse ifchain for minor efficiency.Since the curve names are mutually exclusive, using
else ifwould skip unnecessary comparisons after a match is found.♻️ Suggested refactor
- if (data_len == 12 && memcmp(data, "edwards25519", data_len) == 0) + if (data_len == 12 && memcmp(data, "edwards25519", 12) == 0) { nid = EVP_PKEY_ED25519; } - - if (data_len == 10 && memcmp(data, "curve25519", 10) == 0) + else if (data_len == 10 && memcmp(data, "curve25519", 10) == 0) { nid = EVP_PKEY_X25519; } - - if (data_len == 10 && memcmp(data, "edwards448", 10) == 0) + else if (data_len == 10 && memcmp(data, "edwards448", 10) == 0) { nid = EVP_PKEY_ED448; } - - if (data_len == 8 && memcmp(data, "curve448", 8) == 0) + else if (data_len == 8 && memcmp(data, "curve448", 8) == 0) { nid = EVP_PKEY_X448; }Note: Also changed line 211's
memcmpthird argument fromdata_lento12for consistency with the other comparisons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLUtil.cpp` around lines 211 - 229, The sequence of independent if statements comparing curve name bytes is inefficient and can cause redundant checks; change the four separate ifs that compare data/data_len and assign nid (currently checking "edwards25519", "curve25519", "edwards448", "curve448") into an else-if chain so once a match is found subsequent comparisons are skipped, and also make the first memcmp call use a fixed length 12 (instead of data_len) to match the other comparisons; update the conditions referencing data, data_len and assign EVP_PKEY_ED25519, EVP_PKEY_X25519, EVP_PKEY_ED448, EVP_PKEY_X448 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/crypto/OSSLUtil.cpp`:
- Around line 211-229: The sequence of independent if statements comparing curve
name bytes is inefficient and can cause redundant checks; change the four
separate ifs that compare data/data_len and assign nid (currently checking
"edwards25519", "curve25519", "edwards448", "curve448") into an else-if chain so
once a match is found subsequent comparisons are skipped, and also make the
first memcmp call use a fixed length 12 (instead of data_len) to match the other
comparisons; update the conditions referencing data, data_len and assign
EVP_PKEY_ED25519, EVP_PKEY_X25519, EVP_PKEY_ED448, EVP_PKEY_X448 accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 167f5af6-7c32-4e3c-952a-1aeb5443606d
📒 Files selected for processing (1)
src/lib/crypto/OSSLUtil.cpp
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
Good catch of the mem leak! Just a minor suggestion of not repeating those data_len checks which I'm not 100% sure every compiler optimizes out...
olszomal
force-pushed
the
fix-eddsa-leak
branch
from
f4235fa to
799586c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/OSSLUtil.cpp`:
- Around line 198-236: The branch that decodes a PrintableString into curve_name
(via d2i_ASN1_PRINTABLESTRING) frees curve_name only on matched returns, leaking
curve_name on the fall-through return nid; path; fix by mirroring the OID
branch: do not return inside each match, instead set nid to the appropriate
EVP_PKEY_* (e.g., EVP_PKEY_ED25519, EVP_PKEY_X25519, EVP_PKEY_ED448,
EVP_PKEY_X448) when memcmp matches, then after the checks call
ASN1_PRINTABLESTRING_free(curve_name) once and return nid so every exit path
frees curve_name; ensure curve_name is freed before any return and unchanged
behavior when d2i_ASN1_PRINTABLESTRING returns NULL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37f8c77d-e6a1-4e6b-a625-ad3016d4ebb0
📒 Files selected for processing (1)
src/lib/crypto/OSSLUtil.cpp
olszomal
force-pushed
the
fix-eddsa-leak
branch
from
799586c to
be57521
Compare
|
Oh sorry I was just going through PR in order from the latest and merged #863 . I was thinking that it is familiar but didn't realise that you fixed the same thing. This actually still has got that curve check and slightly more optimal comparison so if you could strip what got merged and keep what's left, that would be awesome. |
olszomal
force-pushed
the
fix-eddsa-leak
branch
from
be57521 to
f9c0b97
Compare
|
No worries. I rebased onto #863, removed the already merged parts, and kept the remaining improvements. Should be clean now. |
2 participants
Fix a small memory leak in
OSSL::byteString2oid()when CKA_EC_PARAMS is decoded as ASN1_PRINTABLESTRING for EdDSA keys.Also replace
strcmp()on ASN.1 string data with length-awarememcmp()usingASN1_STRING_get0_data()andASN1_STRING_length(), as ASN.1 strings are not guaranteed to be NUL-terminated.For consistency, also free the temporary ASN1_OBJECT in the OID path.
Detected via Valgrind (ASN1_PRINTABLESTRING leak during C_SignInit with EdDSA keys).
Summary by CodeRabbit