Skip to content
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

RecordParser may grow the initial buffer until the first event is emitted #4960

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

tsegismont
Copy link
Contributor

See #4811

This can be a problem if the received Buffer is backed by a Netty ByteBuff with low capacity.

Also, we shouldn't assume we can modify input.

…tted

See eclipse-vertx#4811

This can be a problem if the received Buffer is backed by a Netty ByteBuff with low capacity.
Also, we shouldn't assume we can modify input.

To reproduce the issue, the client and the server should communicate over an encrypted connection.
In this case, VertxHandler#safeHandler does not copy the received buffer because it's not pooled.
These buffers are created at the encryption support layer and have a limited, low capacity.

The tests in this commit don't pass without the fix in RecordParser (copy initial buffer).

Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont
Copy link
Contributor Author

The test failures are due to:

java.lang.UnsupportedOperationException: OpenJdkSelfSignedCertGenerator not supported on the used JDK version

🤦‍♂️

Instead of a self-signed certificate created at runtime.

Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont tsegismont removed the request for review from vietj November 22, 2023 09:20
@tsegismont
Copy link
Contributor Author

As agreed off-list, will merge this with an extra commit adding a comment in the RecordParser implementation.

Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont tsegismont merged commit bf67c78 into eclipse-vertx:master Nov 22, 2023
@tsegismont tsegismont deleted the issues/4811 branch November 22, 2023 10:06
tsegismont added a commit that referenced this pull request Nov 22, 2023
…tted (#4960)

* RecordParser may grow the initial buffer until the first event is emitted

See #4811

This can be a problem if the received Buffer is backed by a Netty ByteBuff with low capacity.
Also, we shouldn't assume we can modify input.

To reproduce the issue, the client and the server should communicate over an encrypted connection.
In this case, VertxHandler#safeHandler does not copy the received buffer because it's not pooled.
These buffers are created at the encryption support layer and have a limited, low capacity.

The tests in this commit don't pass without the fix in RecordParser (copy initial buffer).

Signed-off-by: Thomas Segismont <[email protected]>

* ClientResponseParserTest: use SERVER_PEM

Instead of a self-signed certificate created at runtime.

Signed-off-by: Thomas Segismont <[email protected]>

* Add comment in RecordParserImpl

Signed-off-by: Thomas Segismont <[email protected]>

---------

Signed-off-by: Thomas Segismont <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant