apacheGH-403: Use 4-byte SFTP file handles by default

Change the default size for SFTP file handles to 4 bytes, and change
the implementation such that it produces collision-free 4-byte handles.

The algorithm for longer handles is left unchanged.

Using smaller SFTP file handles wastes less space in the SSH packets,
which can thus be used to send actual data. OpenSSH also uses 4-byte
SFTP file handles; using the same size also helps avoid strange bugs in
SFTP clients that might not always handle larger file handles correctly.

Bug: apache#403

CHANGES.md

@@ -56,6 +56,12 @@ handle size was thus 32 bytes.
 
 This has been fixed in this version.
 
+Additionally, the default setting for the size of file handles has been changed
+from 16 to 4 bytes. OpenSSH also uses 4-byte SFTP file handles. Using the same
+size not only means that there is a little more space left in SSH packets for
+actual data transfer. It also completely avoids the WS_FTP bug mentioned in
+[GH-403](https://github.com/apache/mina-sshd/issues/403).
+
 ## Potential compatibility issues
 
 ### Server-side SFTP file handle encoding

sshd-sftp/src/main/java/org/apache/sshd/sftp/SftpModuleProperties.java

@@ -208,8 +208,8 @@ public final class SftpModuleProperties {
  public static final Property<Integer> MAX_OPEN_HANDLES_PER_SESSION
  = Property.integer("max-open-handles-per-session", Integer.MAX_VALUE - 1);
 
- public static final int MIN_FILE_HANDLE_SIZE = 4; // ~uint32
- public static final int DEFAULT_FILE_HANDLE_SIZE = 16;
+ public static final int MIN_FILE_HANDLE_SIZE = Integer.BYTES; // ~uint32
+ public static final int DEFAULT_FILE_HANDLE_SIZE = MIN_FILE_HANDLE_SIZE;
  public static final int MAX_FILE_HANDLE_SIZE = 64; // ~sha512
 
  /**

sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java

@@ -34,8 +34,11 @@
 import java.nio.file.NoSuchFileException;
 import java.nio.file.NotDirectoryException;
 import java.nio.file.Path;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.Deque;
+import java.util.LinkedList;
 import java.util.Map;
 import java.util.Objects;
 import java.util.TreeMap;
@@ -108,6 +111,7 @@ public class SftpSubsystem
  protected Environment env;
  protected Random randomizer;
  protected int maxHandleCount = Integer.MAX_VALUE - 1;
+ protected Deque<String> unusedHandles;
  protected int fileHandleSize = SftpModuleProperties.DEFAULT_FILE_HANDLE_SIZE;
  protected int maxFileHandleRounds = SftpModuleProperties.DEFAULT_FILE_HANDLE_ROUNDS;
  protected Future<?> pendingFuture;
@@ -212,6 +216,8 @@ protected void initializeSessionRelatedMember(ServerSession session, ChannelSess
  return false;
  };
  channel.setPacketValidator(validator);
+ } else {
+ this.unusedHandles = new LinkedList<>();
  }
  if (workBuf.length < this.fileHandleSize) {
  workBuf = new byte[this.fileHandleSize];
@@ -616,8 +622,7 @@ protected void doCopyData(
  log.debug("doCopyData({})[id={}] SSH_FXP_EXTENDED[{}] read={}[{}]"
  + ", read-offset={}, read-length={}, write={}[{}], write-offset={})",
  getServerSession(), id, SftpConstants.EXT_COPY_DATA,
- Handle.safe(readHandle), rh, readOffset, readLength,
- Handle.safe(writeHandle), wh, writeOffset);
+ Handle.safe(readHandle), rh, readOffset, readLength, Handle.safe(writeHandle), wh, writeOffset);
  }
 
  FileHandle srcHandle = validateHandle(readHandle, rh, FileHandle.class);
@@ -757,7 +762,8 @@ protected void doReadDir(Buffer buffer, int id) throws IOException {
  Boolean indicator = SftpHelper.indicateEndOfNamesList(reply, sftpVersion, session, dh.isDone());
  if (debugEnabled) {
  log.debug("doReadDir({})({})[{}] - sending {} entries - eol={} (SFTP version {})", session,
- Handle.safe(handle), h, count, indicator, sftpVersion);
+ Handle.safe(handle), h,
+ count, indicator, sftpVersion);
  }
  } else {
  // empty directory
@@ -912,7 +918,19 @@ protected int doRead(
 
  @Override
  protected void doClose(int id, String handle) throws IOException {
- Handle h = handles.remove(handle);
+ Handle h;
+ synchronized (handles) {
+ h = handles.remove(handle);
+ if (fileHandleSize == Integer.BYTES) {
+ if (handles.isEmpty()) {
+ // No currently used handles: we may forget about unused handles and start from scratch
+ unusedHandles.clear();
+ } else if (h != null && handle != null) {
+ // If h is null, the handle was invalid
+ unusedHandles.push(handle);
+ }
+ }
+ }
  ServerSession session = getServerSession();
  if (log.isDebugEnabled()) {
  log.debug("doClose({})[id={}] SSH_FXP_CLOSE (handle={}[{}])", session, id, Handle.safe(handle), h);
@@ -979,6 +997,21 @@ protected String generateFileHandle(Path file) throws IOException {
  "Too many open handles: current=" + curHandleCount + ", max.=" + maxHandleCount);
  }
  boolean traceEnabled = log.isTraceEnabled();
+ if (fileHandleSize == Integer.BYTES) {
+ String handle = unusedHandles.poll();
+ if (handle == null) {
+ // No usused previously created handle available: all handles are used and have values in the range of
+ // [0 .. curHandleCount). So we can simply use curHandleCount for the new handle and be sure it is
+ // unique.
+ Arrays.fill(workBuf, (byte) 0);
+ BufferUtils.putUInt(curHandleCount, workBuf);
+ handle = new String(workBuf, 0, Integer.BYTES, StandardCharsets.ISO_8859_1);
+ }
+ if (traceEnabled) {
+ log.trace("generateFileHandle({})[{}] {}", getServerSession(), file, Handle.safe(handle));
+ }
+ return handle;
+ }
  // use several rounds in case the file handle size is relatively small so we might get conflicts
  ServerSession session = getServerSession();
  for (int index = 0; index < maxFileHandleRounds; index++) {

sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java

@@ -116,21 +116,34 @@
 import org.junit.Before;
 import org.junit.FixMethodOrder;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
 
 /**
  * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
  */
 @FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@RunWith(Parameterized.class)
 @SuppressWarnings("checkstyle:MethodCount")
 public class SftpTest extends AbstractSftpClientTestSupport {
  private static final Map<String, OptionalFeature> EXPECTED_EXTENSIONS
  = AbstractSftpSubsystemHelper.DEFAULT_SUPPORTED_CLIENT_EXTENSIONS;
 
  private com.jcraft.jsch.Session session;
 
- public SftpTest() throws IOException {
- super();
+ private final int sftpHandleSize;
+
+ public SftpTest(int handleSize) throws IOException {
+ sftpHandleSize = handleSize;
+ }
+
+ @Parameterized.Parameters(name = "FILE_HANDLE_SIZE {0}")
+ public static List<Integer> getParameters() {
+ List<Integer> result = new ArrayList<>();
+ result.add(Integer.valueOf(4));
+ result.add(Integer.valueOf(16));
+ return result;
  }
 
  @Before
@@ -143,6 +156,7 @@ public void setUp() throws Exception {
  outputDebugMessage("Removed forced version=%s", forced);
  }
 
+ SftpModuleProperties.FILE_HANDLE_SIZE.set(sshd, Integer.valueOf(sftpHandleSize));
  JSch sch = new JSch();
  session = sch.getSession("sshd", TEST_LOCALHOST, port);
  session.setUserInfo(new SimpleUserInfo("sshd"));
@@ -1116,56 +1130,91 @@ public void testWriteCreateAppend() throws Exception {
 
  @Test
  public void testHandleSize() throws Exception {
+ List<? extends SubsystemFactory> factories = sshd.getSubsystemFactories();
+ assertEquals("Mismatched subsystem factories count", 1, GenericUtils.size(factories));
+
+ SubsystemFactory f = factories.get(0);
+ assertObjectInstanceOf("Not an SFTP subsystem factory", SftpSubsystemFactory.class, f);
+
+ SftpSubsystemFactory factory = (SftpSubsystemFactory) f;
+ Set<String> handles = new HashSet<>();
+ SftpEventListener listener = new AbstractSftpEventListenerAdapter() {
+
+ @Override
+ public void open(ServerSession session, String remoteHandle, Handle localHandle) throws IOException {
+ handles.add(remoteHandle);
+ }
+ };
+ factory.addSftpEventListener(listener);
  int handleSize = SftpModuleProperties.FILE_HANDLE_SIZE.getRequired(sshd).intValue();
- Path targetPath = detectTargetFolder();
- Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME,
- getClass().getSimpleName(), getCurrentTestName());
- CommonTestSupportUtils.deleteRecursive(lclSftp);
+ try {
+ Path targetPath = detectTargetFolder();
+ Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME,
+ getClass().getSimpleName(), getCurrentTestName());
+ CommonTestSupportUtils.deleteRecursive(lclSftp);
 
- Path parentPath = targetPath.getParent();
- Path clientFolder = assertHierarchyTargetFolderExists(lclSftp).resolve("client");
+  Path parentPath = targetPath.getParent();
+  Path clientFolder = assertHierarchyTargetFolderExists(lclSftp).resolve("client");
 
- String dir = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, clientFolder);
+  String dir = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, clientFolder);
 
- try (SftpClient sftp = createSingleSessionClient()) {
- sftp.mkdir(dir);
- // Write some files
- byte[] data = "something".getBytes(StandardCharsets.US_ASCII);
- try (CloseableHandle handle = sftp.open(dir + "/first.bin", OpenMode.Write, OpenMode.Create)) {
- assertEquals("Unexpected handle size", handleSize, handle.getIdentifier().length);
- sftp.write(handle, 0, data, 0, data.length);
- }
- try (CloseableHandle handle1 = sftp.open(dir + "/second.bin", OpenMode.Write, OpenMode.Create);
- CloseableHandle handle2 = sftp.open(dir + "/third.bin", OpenMode.Write, OpenMode.Create)) {
- assertEquals("Unexpected handle size", handleSize, handle1.getIdentifier().length);
- assertEquals("Unexpected handle size", handleSize, handle2.getIdentifier().length);
- sftp.write(handle1, 0, data, 0, data.length);
- sftp.write(handle2, 0, data, 0, data.length);
- }
- try (CloseableHandle handle1 = sftp.open(dir + "/fourth.bin", OpenMode.Write, OpenMode.Create)) {
- assertEquals("Unexpected handle size", handleSize, handle1.getIdentifier().length);
- byte[] id2;
- try (CloseableHandle handle2 = sftp.open(dir + "/fifth.bin", OpenMode.Write, OpenMode.Create)) {
- id2 = handle2.getIdentifier();
- assertEquals("Unexpected handle size", handleSize, id2.length);
+ try (SftpClient sftp = createSingleSessionClient()) {
+ sftp.mkdir(dir);
+ // Write some files
+ byte[] data = "something".getBytes(StandardCharsets.US_ASCII);
+ try (CloseableHandle handle = sftp.open(dir + "/first.bin", OpenMode.Write, OpenMode.Create)) {
+ assertEquals("Unexpected handle size", handleSize, handle.getIdentifier().length);
+ sftp.write(handle, 0, data, 0, data.length);
+ }
+ assertEquals("Unexpected number of handles", 1, handles.size());
+ try (CloseableHandle handle1 = sftp.open(dir + "/second.bin", OpenMode.Write, OpenMode.Create);
+ CloseableHandle handle2 = sftp.open(dir + "/third.bin", OpenMode.Write, OpenMode.Create)) {
+ assertEquals("Unexpected handle size", handleSize, handle1.getIdentifier().length);
+ assertEquals("Unexpected handle size", handleSize, handle2.getIdentifier().length);
+ sftp.write(handle1, 0, data, 0, data.length);
  sftp.write(handle2, 0, data, 0, data.length);
  }
- byte[] id3;
- try (CloseableHandle handle3 = sftp.open(dir + "/sixth.bin", OpenMode.Write, OpenMode.Create)) {
- id3 = handle3.getIdentifier();
- assertEquals("Unexpected handle size", handleSize, id3.length);
- sftp.write(handle3, 0, data, 0, data.length);
+ if (handleSize == Integer.BYTES) {
+ assertEquals("Unexpected number of handles", 2, handles.size());
+ } else {
+ assertTrue("Unexpected number of handles", handles.size() <= 3);
  }
- sftp.write(handle1, 0, data, 0, data.length);
- }
- assertArrayEquals("Unexpected data in first.bin", data, Files.readAllBytes(clientFolder.resolve("first.bin")));
- assertArrayEquals("Unexpected data in second.bin", data,
- Files.readAllBytes(clientFolder.resolve("second.bin")));
- assertArrayEquals("Unexpected data in third.bin", data, Files.readAllBytes(clientFolder.resolve("third.bin")));
- assertArrayEquals("Unexpected data in fourth.bin", data,
- Files.readAllBytes(clientFolder.resolve("fourth.bin")));
- assertArrayEquals("Unexpected data in fifth.bin", data, Files.readAllBytes(clientFolder.resolve("fifth.bin")));
- assertArrayEquals("Unexpected data in sixth.bin", data, Files.readAllBytes(clientFolder.resolve("sixth.bin")));
+ try (CloseableHandle handle1 = sftp.open(dir + "/fourth.bin", OpenMode.Write, OpenMode.Create)) {
+ assertEquals("Unexpected handle size", handleSize, handle1.getIdentifier().length);
+ byte[] id2;
+ try (CloseableHandle handle2 = sftp.open(dir + "/fifth.bin", OpenMode.Write, OpenMode.Create)) {
+ id2 = handle2.getIdentifier();
+ assertEquals("Unexpected handle size", handleSize, id2.length);
+ sftp.write(handle2, 0, data, 0, data.length);
+ }
+ byte[] id3;
+ try (CloseableHandle handle3 = sftp.open(dir + "/sixth.bin", OpenMode.Write, OpenMode.Create)) {
+ id3 = handle3.getIdentifier();
+ assertEquals("Unexpected handle size", handleSize, id3.length);
+ if (handleSize == Integer.BYTES) {
+ // Should have been re-used
+ assertArrayEquals("Expected handles to be the same", id2, id3);
+ }
+ sftp.write(handle3, 0, data, 0, data.length);
+ }
+ sftp.write(handle1, 0, data, 0, data.length);
+ }
+ assertArrayEquals("Unexpected data in first.bin", data, Files.readAllBytes(clientFolder.resolve("first.bin")));
+ assertArrayEquals("Unexpected data in second.bin", data,
+ Files.readAllBytes(clientFolder.resolve("second.bin")));
+ assertArrayEquals("Unexpected data in third.bin", data, Files.readAllBytes(clientFolder.resolve("third.bin")));
+ assertArrayEquals("Unexpected data in fourth.bin", data,
+ Files.readAllBytes(clientFolder.resolve("fourth.bin")));
+ assertArrayEquals("Unexpected data in fifth.bin", data, Files.readAllBytes(clientFolder.resolve("fifth.bin")));
+ assertArrayEquals("Unexpected data in sixth.bin", data, Files.readAllBytes(clientFolder.resolve("sixth.bin")));
+ if (handleSize == Integer.BYTES) {
+ assertEquals("Unexpected number of handles", 2, handles.size());
+ } else {
+ assertTrue("Unexpected number of handles", handles.size() <= 6);
+ }
+ }
+ } finally {
+ factory.removeSftpEventListener(listener);
  }
  }