Read smaller frames to workaround OpenSSL bug #5115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakirkham
commented
Jul 24, 2021
distributed/comm/tcp.py
Outdated
| @@ -35,6 +38,7 @@ | |||
|
|
|||
|
|
|||
| MAX_BUFFER_SIZE = MEMORY_LIMIT / 2 | |||
| C_MAX_INT = 256 ** ctypes.sizeof(ctypes.c_int) // 2 - 1 | |||
There was a problem hiding this comment.
AFAICT the limit to what OpenSSL can handle is based on the maximum value of int in C (as used by num bytes in this call). This is implementation specific. So am using ctypes to figure that out.
There was a problem hiding this comment.
Just to back this up, the following C program (on my machine) gives this result
#include <stdio.h>
#include <limits.h>
int main() {
printf("%lu\n", INT_MAX);
return 0;
}2147483647
Using this Python code (on my machine), I get the same result.
import ctypes
print(256 ** ctypes.sizeof(ctypes.c_int) // 2 - 1)There was a problem hiding this comment.
Renamed the variable to C_INT_MAX. Otherwise the code is the same
jakirkham
force-pushed
the
wrk_ssl_bug
branch
2 times, most recently
from
54a734b to
14247d5
Compare
jakirkham
force-pushed
the
wrk_ssl_bug
branch
3 times, most recently
from
420bf97 to
ca30c78
Compare
As older versions of OpenSSL (in particular 1.0.2) have limitations on the size of buffers they can work with, take small views into our larger buffer and read those in instead. This should keep the buffer sizes more manageable for OpenSSL.
|
Looks like the one CI test failure is a known flaky test ( #4957 ) |
jakirkham
changed the title
|
cc @fjetter |
|
@jakirkham if you're confident that this is a good fix I'm more than happy to trust you on it. Please merge at will if so. |
|
Doing the same thing for the |
mrocklin
added a commit
to mrocklin/distributed
that referenced
this pull request
Jul 29, 2021
Supercedes dask#5134 Copying over the summary of that PR Works around the OpenSSL 1.0.2 bug demonstrated in issue ( dask#4538 ), except unlike PR ( dask#5115 ) which did this for reading, this does the same thing for writing. The error may be less likely to show up in the write path (as frames may simply be smaller than this limit). Still it seems like a good idea to protect against OverflowErrors from OpenSSL
3 tasks
jrbourbeau
pushed a commit
that referenced
this pull request
Jul 29, 2021
Supercedes #5134 Copying over the summary of that PR Works around the OpenSSL 1.0.2 bug demonstrated in issue ( #4538 ), except unlike PR ( #5115 ) which did this for reading, this does the same thing for writing. The error may be less likely to show up in the write path (as frames may simply be smaller than this limit). Still it seems like a good idea to protect against OverflowErrors from OpenSSL
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As older versions of OpenSSL (in particular 1.0.2) have limitations on the size of buffers they can work with, take small views into our larger buffer and read those in instead. This should keep the buffer sizes more manageable for OpenSSL.
black distributed/flake8 distributed/isort distributedCloses #4538