Rework memory BIOs and implement BIO_seek (2nd try) #2433

nhatnghiho · 2025-05-22T02:45:56Z

Issues:

Resolves #CryptoAlg-3111

Description of changes:

Same as #2380.

Fixed the state bug that was causing Ruby failures. The failures came from BIO_reset, which tries to "rewind" the R/O buffer to its original state. The old implementation only works when the buffer pointer and read pointer are aligned. But in the new memory BIO, this is not always the case. The buffer pointer (b->data) does not need to align with the read pointer (since we can have a positive bbm->read_off). Our tests didn't cover this because the buffer pointer was synced with read pointer after every test case.

Testing:

Added tests for BIO_reset and BIO_seek functions that attempt to rewind the buffer internally. The tests cover both cases -- when buffer pointer and read pointer are synced and when they are not.

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.

### Issues: Resolves #CryptoAlg-3111 ### Description of changes: This PR creates a wrapper around `BUF_MEM` and adds a new field `off` to indicate the current offset of the read pointer. This enables us to implement `BIO_seek`, which is also in this PR. ### Testing: Unit tests are modified and added to cover more test cases. 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.

github-actions

clang-tidy made some suggestions

github-actions · 2025-05-22T02:50:24Z

crypto/bio/bio_mem.c


 BIO *BIO_new_mem_buf(const void *buf, ossl_ssize_t len) {
  BIO *ret;
  BUF_MEM *b;
-  const size_t size = len < 0 ? strlen((char *)buf) : (size_t)len;
+  BIO_BUF_MEM *bbm;


warning: variable 'bbm' is not initialized [cppcoreguidelines-init-variables]

Suggested change

BIO_BUF_MEM *bbm;

BIO_BUF_MEM *bbm = NULL;

codecov-commenter · 2025-05-22T03:07:31Z

Codecov Report

Attention: Patch coverage is 93.81443% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.89%. Comparing base (f0b4afe) to head (4c2b05d).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/bio/bio_mem.c	90.32%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2433      +/-   ##
==========================================
+ Coverage   78.88%   78.89%   +0.01%     
==========================================
  Files         621      621              
  Lines      108547   108671     +124     
  Branches    15406    15420      +14     
==========================================
+ Hits        85623    85738     +115     
- Misses      22256    22261       +5     
- Partials      668      672       +4

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

crypto/bio/bio_mem.c

This reverts commit d93dde3.

…2466) This reverts commit d93dde3. ### Description of changes: It's not clear why yet, but reverting [this PR](#2433) seems to [fix HAProxy](https://us-west-2.codebuild.aws.amazon.com/project/eyJlbmNyeXB0ZWREYXRhIjoicEtxdUl5RTdsWW95MmtnM0pSWjJmQU92T2RhQUJPN2VzRFdXZTdHOG1FWU1NQ2VwcGtucE92ZlA1Q3pSRHFCWW1VRTFLa3VTN1RWVC96dmsraFl5WXZwSmFha2tOTFZBOEdiWittVlRGKzlBVkRqVk5Gaz0iLCJpdlBhcmFtZXRlclNwZWMiOiJxS01SNWtSdUR0d2NvSUMvIiwibWF0ZXJpYWxTZXRTZXJpYWwiOjF9/build/a0318c48-a852-4f79-8ddb-85f17ce12755). 🤷‍♂️ 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.

Resolves #CryptoAlg-3111 Same as aws#2380. Fixed the state bug that was causing Ruby failures. The failures came from `BIO_reset`, which tries to "rewind" the R/O buffer to its original state. The old implementation only works when the buffer pointer and read pointer are aligned. But in the new memory BIO, this is not always the case. The buffer pointer (`b->data`) does not need to align with the read pointer (since we can have a positive `bbm->read_off`). Our tests didn't cover this because the buffer pointer was synced with read pointer after every test case. Added tests for `BIO_reset` and `BIO_seek` functions that attempt to rewind the buffer internally. The tests cover both cases -- when buffer pointer and read pointer are synced and when they are not. 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.

@m271828

## What's Changed * Add build with hardened flag by @m271828 in #2396 * Openssl tool output ordered by options provided by @justsmth in #2452 * [SCRUTINICE] Remove redundant condition check by @nhatnghiho in #2450 * Support relro in delocator by @torben-hansen in #2455 * Explicitly don't allow buffers aliasing in ctr-drbg implementation by @torben-hansen in #2458 * Remove unused Windows afunix.h by @justsmth in #2461 * Revert "Rework memory BIOs and implement BIO_seek (2nd try) (#2433)" by @justsmth in #2466 * Use max_cert_list for TLSv1.3 NewSessionTicket by @skmcgrail in #2453 * ML-KEM memory safety by @m271828 in #2263 * Simplify Compiler CI jobs by @justsmth in #2430 * Improve support for multilib-style distros in our test scripts by @justsmth in #2467 * Fix Ruby mainline and nginx CI by @samuel40791765 in #2460 * Add hardened build back in by @m271828 in #2474 * Fix OCSP integration test failures by @samuel40791765 in #2480 * Fix some theoretical missing earlyclobber markers in inline assembly by @torben-hansen in #2477 * Simplify sshkdf and kbkdf by @torben-hansen in #2478 * Run 3p module tests on python 3.13, add patch for 3.14 by @WillChilds-Klein in #2476 * [UPSTREAM] Fix BIO_eof for BIO pairs by @justsmth in #2440 * Fix service indicator in HKDF, more paranoid zeroization, and simplify logic by @torben-hansen in #2482 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.

### Issues: Resolves #CryptoAlg-3111 ### Description of changes: Same as #2433. Fixed the bug that was causing HAProxy failures. The previous tries attempted to disallow passing empty memory BIOs into BIO functions (such as `BIO_reset`) by treating that as errors. HAProxy didn't expect this error, hence the failures. This PR reverted that behavior. PQ-TLS integration test also started failing consistently after my first BIO changes but investigation showed no memory BIO functions were used in PQ integ test. Concluded that this change is now safe to merge. ### Testing: No additional testing needed 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.

nhatnghiho and others added 2 commits May 21, 2025 22:57

Fix buffer rewind/reset bug in R/O mode

fb9eef4

nhatnghiho requested a review from a team as a code owner May 22, 2025 02:45

github-actions bot reviewed May 22, 2025

View reviewed changes

justsmth previously approved these changes May 22, 2025

View reviewed changes

Minor code cleanup

4c2b05d

nhatnghiho dismissed justsmth’s stale review via 4c2b05d May 22, 2025 19:18

skmcgrail reviewed May 22, 2025

View reviewed changes

crypto/bio/bio_mem.c Show resolved Hide resolved

skmcgrail approved these changes May 22, 2025

View reviewed changes

justsmth approved these changes May 23, 2025

View reviewed changes

justsmth merged commit d93dde3 into aws:main May 23, 2025
113 of 116 checks passed

justsmth added a commit to justsmth/aws-lc that referenced this pull request Jun 5, 2025

Revert "Rework memory BIOs and implement BIO_seek (2nd try) (aws#2433)"

82a2627

This reverts commit d93dde3.

justsmth mentioned this pull request Jun 5, 2025

Revert "Rework memory BIOs and implement BIO_seek (2nd try) (#2433)" #2466

Merged

justsmth mentioned this pull request Jun 6, 2025

Prepare v1.53.0 #2471

Merged

nhatnghiho mentioned this pull request Jun 6, 2025

Rework memory BIOs and implement BIO_seek (3rd try) #2472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rework memory BIOs and implement BIO_seek (2nd try) #2433

Rework memory BIOs and implement BIO_seek (2nd try) #2433

Uh oh!

nhatnghiho commented May 22, 2025 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot May 22, 2025

Uh oh!

codecov-commenter commented May 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rework memory BIOs and implement BIO_seek (2nd try) #2433

Rework memory BIOs and implement BIO_seek (2nd try) #2433

Uh oh!

Conversation

nhatnghiho commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

Description of changes:

Testing:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 22, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nhatnghiho commented May 22, 2025 •

edited

Loading

codecov-commenter commented May 22, 2025 •

edited

Loading