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

Make BIO_get_mem_data a function again #2246

Merged
merged 1 commit into from
Mar 6, 2025
Merged

Conversation

skmcgrail
Copy link
Member

Description of changes:

Can't track it down the historical reason why, but we have BIO_get_mem_data defined as a preprocessor macro, but BoringSSL does not. This creates an annoying compatibility issue for rust-openssl, and has forced us to implement the function properly in our aws-lc-sys crates in Rust. This redefines the function to have a concrete definition.

As a side-note, the macro was technically wrong as it was casting contents to a char * when it was actually char **, but this wasn't problematic per-se since it was being passed into a void * argument. If you look at the ctrl function definition you can see that it is indeed treated as a char ** pointer.

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.

@skmcgrail skmcgrail requested a review from a team as a code owner March 5, 2025 23:56
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.05%. Comparing base (1d8b807) to head (ecf52ea).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2246      +/-   ##
==========================================
- Coverage   79.06%   79.05%   -0.02%     
==========================================
  Files         612      612              
  Lines      106513   106515       +2     
  Branches    15052    15053       +1     
==========================================
- Hits        84216    84204      -12     
- Misses      21645    21659      +14     
  Partials      652      652              

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@samuel40791765 samuel40791765 left a comment

Choose a reason for hiding this comment

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

Original Context: I was forced to change this to a macro because mySQL was using the function improperly. I submitted a change to upstream mysql to fix this. They've accepted it and it's in versions after mysql-8.3.0: https://bugs.mysql.com/bug.php?id=112846
We've agreed to use the referenced commit as a patch for any possible consumers of MySQL versions earlier than 8.3.

@skmcgrail skmcgrail merged commit 5826a07 into aws:main Mar 6, 2025
110 of 114 checks passed
@skmcgrail skmcgrail deleted the compat branch March 6, 2025 18:39
@skmcgrail skmcgrail mentioned this pull request Mar 6, 2025
skmcgrail added a commit that referenced this pull request Mar 6, 2025
## What's Changed
* Move OCSP ASN1 type functions to public header by @skmcgrail in
#2239
* Make BIO_get_mem_data a function again by @skmcgrail in
#2246

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.
skmcgrail added a commit to skmcgrail/aws-lc that referenced this pull request Mar 6, 2025
### Description of changes:
Can't track it down the historical reason why, but we have
`BIO_get_mem_data` defined as a preprocessor macro, but BoringSSL does
not. This creates an annoying compatibility issue for rust-openssl, and
has forced us to implement the function properly in our aws-lc-sys
crates in Rust. This redefines the function to have a concrete
definition.

As a side-note, the macro was technically wrong as it was casting
contents to a `char *` when it was actually `char **`, but this wasn't
problematic per-se since it was being passed into a `void *` argument.
If you look at the ctrl function definition you can see that it is
indeed treated as a `char **` pointer.

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.

(cherry picked from commit 5826a07)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants