[SSHD-1287] SFTP: better default buffer size handling

Use the local window's packet size for reading, and the remote window's packet size for writing (both minus a bit for protocol overhead) as the default SFTP buffer size. Fix SftpInputStreamAsync to also work for buffer sizes greater than twice what the server will ever return. Previously, the stream read data from the wrong offset in that case.
apache · Oct 31, 2022 · 6d0ef48 · 6d0ef48
1 parent b19a3ed
commit 6d0ef48
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 25 deletions.
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java
@@ -1172,7 +1172,7 @@ public InputStream read(String path, int bufferSize, Collection<OpenMode> mode)
         if (bufferSize <= 0) {
             bufferSize = getReadBufferSize();
         }
-        if (bufferSize < MIN_WRITE_BUFFER_SIZE) {
+        if (bufferSize < MIN_READ_BUFFER_SIZE) {
             throw new IllegalArgumentException("Insufficient read buffer size: " + bufferSize + ", min.="
                                                + MIN_READ_BUFFER_SIZE);
         }
@@ -1184,12 +1184,6 @@ public InputStream read(String path, int bufferSize, Collection<OpenMode> mode)
         return new SftpInputStreamAsync(this, bufferSize, path, mode);
     }
 
-    @Override
-    public InputStream read(String path, Collection<OpenMode> mode) throws IOException {
-        int packetSize = (int) getChannel().getRemoteWindow().getPacketSize();
-        return read(path, packetSize, mode);
-    }
-
     @Override
     public OutputStream write(String path, int bufferSize, Collection<OpenMode> mode) throws IOException {
         if (bufferSize <= 0) {
@@ -1207,18 +1201,12 @@ public OutputStream write(String path, int bufferSize, Collection<OpenMode> mode
         return new SftpOutputStreamAsync(this, bufferSize, path, mode);
     }
 
-    @Override
-    public OutputStream write(String path, Collection<OpenMode> mode) throws IOException {
-        int packetSize = (int) getChannel().getRemoteWindow().getPacketSize();
-        return write(path, packetSize, mode);
-    }
-
     protected int getReadBufferSize() {
         return (int) getClientChannel().getLocalWindow().getPacketSize() - 13;
     }
 
     protected int getWriteBufferSize() {
-        return (int) getClientChannel().getLocalWindow().getPacketSize() - 13;
+        return (int) getClientChannel().getRemoteWindow().getPacketSize() - 13;
     }
 
 }
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpInputStreamAsync.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpInputStreamAsync.java
@@ -281,13 +281,15 @@ protected void fillData() throws IOException {
             SftpClient client = getClient();
             int cur = 0;
             while (cur < nb) {
-                int dlen = client.read(handle, clientOffset, data, cur, nb - cur, eof);
+                int dlen = client.read(handle, clientOffset + cur, data, cur, nb - cur, eof);
+                if (dlen > 0) {
+                    cur += dlen;
+                }
                 Boolean eofSignal = eof.getAndSet(null);
                 if ((dlen < 0) || ((eofSignal != null) && eofSignal.booleanValue())) {
                     eofIndicator = true;
                     break;
                 }
-                cur += dlen;
             }
 
             if (traceEnabled) {

diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java
@@ -233,17 +233,57 @@ public void testReadBufferLimit() throws Exception {
             for (int index = 0; index < readLen; index++) {
                 byte expByte = expected[index];
                 byte actByte = actual[index];
-                if (expByte != actByte) {
-                    fail("Mismatched values at index=" + index
-                         + ": expected=0x" + Integer.toHexString(expByte & 0xFF)
-                         + ", actual=0x" + Integer.toHexString(actByte & 0xFF));
-                }
+                assertEquals("Mismatched values at index=" + index, Integer.toHexString(expByte & 0xFF),
+                        Integer.toHexString(actByte & 0xFF));
             }
         } finally {
             SftpModuleProperties.MAX_READDATA_PACKET_LENGTH.remove(sshd);
         }
     }
 
+    @Test // see SSHD-1287
+    public void testReadWithLargeBuffer() throws Exception {
+        Path targetPath = detectTargetFolder();
+        Path parentPath = targetPath.getParent();
+        Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
+                getCurrentTestName());
+        Path testFile = assertHierarchyTargetFolderExists(lclSftp).resolve("file.bin");
+        byte[] expected = new byte[1024 * 1024];
+
+        Factory<? extends Random> factory = sshd.getRandomFactory();
+        Random rnd = factory.create();
+        rnd.fill(expected);
+        Files.write(testFile, expected);
+
+        String file = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, testFile);
+        try (SftpClient sftp = createSingleSessionClient()) {
+            byte[] actual = new byte[expected.length];
+            try (InputStream in = sftp.read(file,
+                    2 * SftpModuleProperties.MAX_READDATA_PACKET_LENGTH.getRequiredDefault() + 2048)) {
+                int off = 0;
+                int n = 0;
+                while (off < actual.length) {
+                    n = in.read(actual, off, actual.length - off);
+                    if (n < 0) {
+                        break;
+                    }
+                    off += n;
+                }
+                assertEquals("Short read", actual.length, off);
+                if (n >= 0) {
+                    n = in.read();
+                    assertTrue("Stream not at eof", n < 0);
+                }
+            }
+            for (int index = 0; index < actual.length; index++) {
+                byte expByte = expected[index];
+                byte actByte = actual[index];
+                assertEquals("Mismatched values at index=" + index, Integer.toHexString(expByte & 0xFF),
+                        Integer.toHexString(actByte & 0xFF));
+            }
+        }
+    }
+
     @Test // see SSHD-1288
     public void testReadWriteDownload() throws Exception {
         Assume.assumeTrue("Not sure appending to a file opened for reading works on Windows",
@@ -281,10 +321,8 @@ public void testReadWriteDownload() throws Exception {
                 }
                 byte expByte = expected[j];
                 byte actByte = actual[i];
-                if (expByte != actByte) {
-                    fail("Mismatched values at index=" + i + ": expected=0x" + Integer.toHexString(expByte & 0xFF)
-                         + ", actual=0x" + Integer.toHexString(actByte & 0xFF));
-                }
+                assertEquals("Mismatched values at index=" + i, Integer.toHexString(expByte & 0xFF),
+                        Integer.toHexString(actByte & 0xFF));
             }
         }
     }