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

add back ASN1_dup with tests #1591

Merged
merged 8 commits into from
Jun 3, 2024
Merged

Conversation

samuel40791765
Copy link
Contributor

Issues:

Addresses CryptoAlg-1717

Description of changes:

ASN1_dup was removed in 419144a in favor of ASN1_Item_dup. This shouldn't normally be called directly, but Ruby happens to consume the API in several instances.
I've optimized the function to allocate memory with i2d directly, instead of the ancient OpenSSL allocate and pass into behavior.
Also added some tests for verification along with a do-not-use warning.

Call-outs:

N/A

Testing:

New tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 78.06%. Comparing base (94e91d9) to head (fb3e2e7).

Files Patch % Lines
crypto/asn1/a_dup.c 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1591      +/-   ##
==========================================
+ Coverage   78.03%   78.06%   +0.02%     
==========================================
  Files         562      562              
  Lines       94558    94600      +42     
  Branches    13576    13575       -1     
==========================================
+ Hits        73790    73848      +58     
+ Misses      20175    20160      -15     
+ Partials      593      592       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto/asn1/a_dup.c Outdated Show resolved Hide resolved
crypto/asn1/a_dup.c Show resolved Hide resolved
crypto/asn1/asn1_test.cc Show resolved Hide resolved
skmcgrail
skmcgrail previously approved these changes May 24, 2024
justsmth
justsmth previously approved these changes May 31, 2024
@samuel40791765
Copy link
Contributor Author

samuel40791765 commented May 31, 2024

Apparently the CFI is still failing with the macros:

/codebuild/output/src1171782177/src/github.com/aws/aws-lc/crypto/asn1/a_dup.c:70:17: runtime error: control flow integrity check for type 'int (const void *, unsigned char **)' failed during indirect function call
(/codebuild/output/src1171782177/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test+0x3afd5e0): note: i2d_ASN1_TYPE defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /codebuild/output/src1171782177/src/github.com/aws/aws-lc/crypto/asn1/a_dup.c:70:17 in 

I reverted to run 4a5be1e against the CFI sanitizers, but apparently it doesn't actually pass the CFI tests either. I'll need to revert 4a5be1e and 0cd2716 to remove the macros and get the CFI build passing.

I think the main issue is that i2d_of_void and d2i_of_void expect a void parameter, but the typical i2d/d2i functions have a specific type assigned as the parameter. Upon closer reexamination, the macros don't seem to solve this issue. The two CI failures in the commits may show more details.

@samuel40791765 samuel40791765 dismissed stale reviews from justsmth and skmcgrail via 09e90b5 May 31, 2024 20:56
This reverts commit 4a5be1e.
@samuel40791765 samuel40791765 merged commit 8258d73 into aws:main Jun 3, 2024
89 of 92 checks passed
@samuel40791765 samuel40791765 deleted the asn1-dup branch June 3, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants