You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/11/25 18:18:00 UTC
[GitHub] [ignite] ptupitsyn opened a new pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
ptupitsyn opened a new pull request #9610:
URL: https://github.com/apache/ignite/pull/9610
* Check first 3 bytes of the handshake message without allocating the buffer, drop connection in case of garbage data.
* Add 1 MiB handshake size limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r757599200
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
We should pick a limit and stick to it. Current limit is essentially 2 GiB, but having a password or user attributes this large does not make sense, and may cause all sorts of performance issues.
If 1 MiB is too low, let's pick 10 MiB or 64 MiB, but no more than that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r757599200
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
We should pick a limit and stick to it. Update documentation and state that everything else is not supported.
Current limit is essentially **2 GiB**, but having a password or user attributes this large does not make sense, and may cause all sorts of performance issues.
If **1 MiB** is too low, let's pick **10 MiB** or **64 MiB**, but no more than that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] isapego commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
isapego commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r757583818
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
I'm not sure I like this solution as when we already implemented similia solution in the past and very soon got a complaint from a user that had really big password. Also, since that time we have introduced user attributes, which also can be pretty large and I can easily imagine handshake exceeding 1 MB. If we want to limit handshake size at least lets make it really big.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r759190432
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
Changed it to 64 MiB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] DirectXceriD commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
DirectXceriD commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r759155185
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
64Mb won't cause OOM (most likely), and it will cover case with large handshake payload, I'm good with that. @ptupitsyn @isapego thanks for letting me know, let's proceed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn merged pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
ptupitsyn merged pull request #9610:
URL: https://github.com/apache/ignite/pull/9610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] isapego commented on a change in pull request #9610: IGNITE-15921 Thin client: drop connection on invalid handshake without allocating buffer
Posted by GitBox <gi...@apache.org>.
isapego commented on a change in pull request #9610:
URL: https://github.com/apache/ignite/pull/9610#discussion_r757622265
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientMessage.java
##########
@@ -35,6 +35,18 @@
/** */
private static final long serialVersionUID = -4609408156037304495L;
+ /** */
+ private static final int MAX_HANDSHAKE_SIZE = 1024 * 1024;
Review comment:
64MB seems OK to me, but let's see what @DirectXceriD and @slukyano think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org