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 support for BIO_read/write_ex; Update MySQL CI to 8.4; #1568

Merged
merged 7 commits into from
May 8, 2024

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented May 1, 2024

Description of changes:

MySQL 8.4 has been released, this updates our CI to run against the latest version. This time we only need two missing APIs from OpenSSL.

  • BIO_read_ex
  • BIO_write_ex

Call-outs:

  • Removed disabled tests were removed from mysql's test suite, so I removed them from mysql_run_tests
  • Original patch needed to be updated, new patch is needed for int to size_t mismatch for BIO_pending
    • I considered switching our BIO_pending to int instead, but our version has been using the size_t signature for a decade now, so this doesn't seem easily convertible.

Testing:

  • New tests for BIO_read/write_ex
  • Update MySQL build for 8.4

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.

crypto/bio/bio.c Outdated Show resolved Hide resolved
crypto/bio/bio.c Outdated Show resolved Hide resolved
@samuel40791765 samuel40791765 force-pushed the mysql-8.4 branch 4 times, most recently from 2af6dfb to 0f199fc Compare May 2, 2024 18:15
skmcgrail
skmcgrail previously approved these changes May 2, 2024
Copy link
Contributor

@andrewhop andrewhop left a comment

Choose a reason for hiding this comment

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

Do we need to do anything to support #1333 and handle openssl/openssl#8208?

crypto/bio/bio_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Do we need to do anything to support #1333 and handle openssl/openssl#8208?

  1. For Implement SSL_MODE_AUTO_RETRY #1333, I'm not sure how SSL_MODE_AUTO_RETRY comes into play here since these are BIO APis we're adding here. But I've reverted the change in 171ee7a since it wasn't needed anymore. SSL_MODE_AUTO_RETRY is also default behavior which we've documented.

  2. For BIO_read_ex does not distinguish EOF from failure openssl/openssl#8208, really good catch I didn't notice this issue.. Luckily nobody other than MySQL 8.4 is dependent on us for this yet, but it'd be better to implement the right behavior from the get go.
    My only concern would be folks depending on the non-existent eof support BIO_read_ex has right now. But judging from the comments in the thread it seems like BIO_read_ex is unusable right now because of this, so it seems unlikely. I'll implement the suggestion from BIO_read_ex does not distinguish EOF from failure openssl/openssl#8208 (comment) and document the discrepancy. Will also add tests.

crypto/bio/bio_test.cc Outdated Show resolved Hide resolved
@samuel40791765
Copy link
Contributor Author

We decided to pivot and align with OpenSSL's original behavior instead. Misaligning the behavior will only create more gaps with OpenSSL/AWS-LC with no real additional benefit. This will only create more confusion further down the road with future integration projects. It's also very unlikely that OpenSSL will change the current behavior of BIO_read_ex for the sake of backwards compatibility.
I've added documentation to discourage usage of the API and linked to the original OpenSSL issue.

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 77.91%. Comparing base (2dc5184) to head (3edd87c).

Files Patch % Lines
crypto/bio/bio.c 85.71% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1568      +/-   ##
==========================================
- Coverage   77.92%   77.91%   -0.01%     
==========================================
  Files         560      560              
  Lines       94583    94613      +30     
  Branches    13610    13617       +7     
==========================================
+ Hits        73700    73721      +21     
- Misses      20288    20296       +8     
- Partials      595      596       +1     

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

@samuel40791765 samuel40791765 merged commit 8ae155b into aws:main May 8, 2024
79 checks passed
@samuel40791765 samuel40791765 deleted the mysql-8.4 branch May 8, 2024 20:53
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