[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX

[GitBox <gi...@apache.org>]

lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668893454



##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -696,7 +702,18 @@ protected void handleKexInit(Buffer buffer) throws Exception {
     protected void doKexNegotiation() throws Exception {
         if (kexState.compareAndSet(KexState.DONE, KexState.RUN)) {
             sendKexInit();
-        } else if (!kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+        } else if (kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+            DefaultKeyExchangeFuture initFuture;
+            synchronized (kexState) {
+                initFuture = kexInitializedFuture;
+                if (initFuture == null) {
+                    initFuture = new DefaultKeyExchangeFuture(toString(), kexLock);
+                    kexInitializedFuture = initFuture;
+                }
+            }
+
+            initFuture.await();

Review comment:
       I am not comfortable with an infinite wait - just in case some deadlock/bug occurs. Let's define a `CoreModuleProperties#MAX_KEX_INIT_WAIT_TIMEOUT` and use it here (I think ~30 seconds as default value should do, but you are welcome to use some other value). If it expires then throw an exception (which will close the session(.

##########
File path: sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTransferTest.java
##########
@@ -23,22 +23,39 @@
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
+import java.util.List;
 
 import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.core.CoreModuleProperties;
 import org.apache.sshd.sftp.client.fs.SftpFileSystem;
 import org.junit.FixMethodOrder;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
 
 @FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@RunWith(Parameterized.class)
 public class SftpTransferTest extends AbstractSftpClientTestSupport {
 
     private static final int BUFFER_SIZE = 8192;
 
-    public SftpTransferTest() throws IOException {
+    private final long rekeyBlockSize;
+
+    public SftpTransferTest(long rekeyBlockSize) throws IOException {
         super();

Review comment:
       Please remove the `super()` call - the coding convention is that if there is no other statement in the constructor, then we call `super()` explicitly. Otherwise (i.e., more statements) we drop it.




-- 
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: dev-unsubscribe@mina.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org