-
Notifications
You must be signed in to change notification settings - Fork 833
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 WOLFSSL_FORCE_AUTO_RETRY option: force retrying of network reads #4599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific open source project this fix is being made for?
src/internal.c
Outdated
@@ -65,6 +65,12 @@ | |||
* may be received by a client. To support detecting this, peek will | |||
* return WOLFSSL_ERROR_WANT_READ. | |||
* This define turns off this behaviour. | |||
* WOLFSSL_FORCE_AUTO_RETRY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So defining this macro forces the network reads to behave as blocking? Is there a reason not to implement runtime support using the SSL_MODE_AUTO_RETRY
option? Also can you check review the code comment at ssl.h:2136 for SSL_MODE_AUTO_RETRY
? It seems wrong.
SSL_MODE_AUTO_RETRY = 3, /* wolfSSL default is to block with blocking io and auto retry */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep this PR minimum but if you would prefer a runtime solution, I can add the necessary API to make that work. Still, I don't think that this is a good default for wolfSSL. If you want me to implement a runtime solution, I'll make the SSL_MODE_AUTO_RETRY
option on by default only with OPENSSL_COMPATIBLE_DEFAULTS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the SSL_MODE_AUTO_RETRY
comment to match the real behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment fixed.
src/internal.c
Outdated
@@ -65,6 +65,12 @@ | |||
* may be received by a client. To support detecting this, peek will | |||
* return WOLFSSL_ERROR_WANT_READ. | |||
* This define turns off this behaviour. | |||
* WOLFSSL_FORCE_AUTO_RETRY | |||
* This will force wolfSSL to always retry to read when it receives a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option will force a wolfSSL read to behave as blocking.
True, but fetchmail no longer requires it after recent updates to its TLS code. To be specific for people who find this thread:
|
dd48e71
to
afa6237
Compare
I changed this to set |
Addresses issue #4593