-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Android] Improve SslStream PAL buffer resizing #104726
[Android] Improve SslStream PAL buffer resizing #104726
Conversation
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
int32_t arraySize = length > remaining ? remaining : length; | ||
jbyteArray data = make_java_byte_array(env, arraySize); | ||
// data.setByteArrayRegion(0, length, buffer); | ||
jbyteArray data = make_java_byte_array(env, length); |
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 thought about capping the size of the data
buffer to the max payload size of one TLS frame and keep copying just a subset of the data as we did previously, but the SslStream is already chunking the data (StreamSizes.Default == 32,768
).
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
LGTM
int32_t arraySize = length > remaining ? remaining : length; | ||
jbyteArray data = make_java_byte_array(env, arraySize); | ||
// data.setByteArrayRegion(0, length, buffer); | ||
jbyteArray data = make_java_byte_array(env, length); |
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.
Do we need to allocate a java array here or can we use NIO? appOutBuffer
is a ByteBuffer
. ByteBuffer
has a put
that accepts another ByteBuffer
.
We can create a ByteBuffer
over buffer
with jobject bufferByteBuffer = (*env)->NewDirectByteBuffer(env, buffer, length);
Then we can put
that in to appOutBuffer. That remove an allocation and a copy from a handshake.
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.
Just a thought, can be done as a follow up.
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.
Thanks for the suggestion, I'll give it a try.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
There are failing runtime-extra-platforms tests, but they are all unrelated:
|
Our current implementation of the
AndroidCryptoNative_SSLStreamWrite
doesn't correctly handle buffer overflows when calling thewrap
method. This isn't an issue because we always create buffers large enough to fit the wrapped data. The problem is that we are splitting data into many small chunks instead of sending fewer larger TLS frames and reducing overhead. This also affects certain protocols such as MS-TDS as reported in #95295.Closes #95295
/cc @rzikm @wfurt