Skip to content

aws-lc: do not use large buffer#18434

Closed
icing wants to merge 3 commits intocurl:masterfrom
icing:awslc-buffering-tweak
Closed

aws-lc: do not use large buffer#18434
icing wants to merge 3 commits intocurl:masterfrom
icing:awslc-buffering-tweak

Conversation

@icing
Copy link
Contributor

@icing icing commented Aug 29, 2025

test_10_08, uploading larger files for a h2 proxy, sporadically fails with a decrpytion error on received data in AWS-LC. The frequency can be increased by simulated network receive blocks.

Not setting a 4 * TLS record sized buffer, leaving AWS-LC at its default buffer size seems to mitigate this problem.

@github-actions github-actions bot added the tests label Aug 29, 2025
@icing icing added the TLS label Aug 29, 2025
@icing icing mentioned this pull request Aug 29, 2025
@vszakats
Copy link
Member

vszakats commented Aug 29, 2025

This feature has been disabled for BoringSSL since the original
commit, though for a different reason. I suggest merging the
exclusion with AWS-LC and move the comment there to explain
the reasons. To join these two build variants and simplify a bit:

--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -121,8 +121,12 @@
 static void ossl_provider_cleanup(struct Curl_easy *data);
 #endif
 
+/* BoringSSL is missing this API.
+   AWS-LC seems to run into decryption failures with large buffers.
+   Sporadic failures in test_10_08 with h2 proxy uploads, increased
+   frequency with CURL_DBG_SOCK_RBLOCK=50. */
 #if OPENSSL_VERSION_NUMBER >= 0x10100000L && \
-  !defined(LIBRESSL_VERSION_NUMBER) && !defined(OPENSSL_IS_BORINGSSL)
+  !defined(LIBRESSL_VERSION_NUMBER) && !defined(HAVE_BORINGSSL_LIKE)
 #define HAVE_SSL_CTX_SET_DEFAULT_READ_BUFFER_LEN 1
 #endif

Added to AWS-LC via:
aws/aws-lc@0fb67dd
aws/aws-lc#1517

@icing
Copy link
Contributor Author

icing commented Aug 30, 2025

This feature has been disabled for BoringSSL since the original commit, though for a different reason. I suggest merging the exclusion with AWS-LC and move the comment there to explain the reasons.

Agreed, done.

Comment on lines 131 to 132
Copy link
Member

@vszakats vszakats Aug 30, 2025

Choose a reason for hiding this comment

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

Suggested change
!defined(LIBRESSL_VERSION_NUMBER) && !defined(OPENSSL_IS_BORINGSSL) && \
!defined(OPENSSL_IS_AWSLC)
!defined(LIBRESSL_VERSION_NUMBER) && !defined(HAVE_BORINGSSL_LIKE)

HAVE_BORINGSSL_LIKE covers both BoringSSL-like backends (in lib/vtls), added in: 0be7f38

Save a couple exceptions these two backends use the same codepath in curl.

Copy link
Member

Choose a reason for hiding this comment

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

Since it is disabled for different reasons for the two libraries, maybe keeping the conditions for both makes it easier to change one of them if their individual condition changes?

Copy link
Member

@vszakats vszakats Aug 31, 2025

Choose a reason for hiding this comment

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

It can be done when they will be different in the future IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the disable is for different reasons, I think checking the separate defines is better for maintenance.

test_10_08, uploading larger files for a h2 proxy, sporadically fails
with a decrpytion error on received data in AWS-LC. The frequency can
be increased by simulated network receive blocks.

Not setting a 4 * TLS record sized buffer, leaving AWS-LC at its
default buffer size seems to mitigate this problem.
@icing icing force-pushed the awslc-buffering-tweak branch from 22c83e8 to 25b192e Compare September 1, 2025 16:07
@icing icing requested a review from vszakats September 1, 2025 16:10
@bagder bagder closed this in e65dc7f Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants

Comments