MINA SSHD
  1. MINA SSHD
  2. SSHD-120

list 1500 files in one directory not working for mindterm sftp client

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5.0
    • Fix Version/s: 0.7.0
    • Labels:
      None
    • Environment:
      windows xp professional sun jre 1.5
      windows 2003 server sun jre 1.5
      mindterm 3.4 ftp2sftp

      Description

      mindterm is used in ftp2sftp mode
      trying to list files in a directory "manyfiles" with > 1500 files doesn't work on the "apache mina sshd"
      when doing the following operation it doesn't work. In the "manyfiles" dir there are >1500 files.

      mindterm can list files in a directory with < 50 files
      mindterm can list files on a opensshd server with a directory with > 1500 files, therefore we think the error is in the "apache mina sshd" and not in mindterm, at least there is some incompatibility...

      ftp> o localhost 2120
      Connected to WKE198515.u-dom1.u-ssi.net.
      220 FTPToSFTPProxy use 'ssh2-user@ssh2-host[:port]' as your username or ;ssh2-us
      er;keyfile;10@ssh2-host[:port]
      User (WKE198515.u-dom1.u-ssi.net:(none)): a@localhost:2222
      331 Password required for a@localhost:2222.
      Password:
      230 User a@localhost:2222 logged in.
      ftp> cd ../../../tst/manyfiles
      250 CWD command successful.
      ftp> ls
      200 PORT command successful.
      150 Opening ASCII mode data connection for nlst.
      Connection closed by remote host.

        Activity

        Otto Frost created issue -
        Otto Frost made changes -
        Field Original Value New Value
        Description mindterm is used in ftp2sftp mode
        trying to list files in a directory "manyfiles" with > 1500 files doesn't work on the "apache mina sshd"
        when doing the following operation it doesn't work. In the "manyfiles" dir there are >1500 files.

        mindterm can list files in a directory with < 50 files
        mindterm can list files on a opensshd server with a directory with > 1500 files, therefore we think the error is in the "apache mina sshd" and not in mindterm, at least there is some incompatibility...


        ftp> o localhost 2120
        Connected to WKE198515.u-dom1.u-ssi.net.
        220 FTPToSFTPProxy use 'ssh2-user@ssh2-host[:port]' as your username or ;ssh2-us
        er;keyfile;10@ssh2-host[:port]
        User (WKE198515.u-dom1.u-ssi.net:(none)): a@localhost:2222
        331 Password required for a@localhost:2222.
        Password:
        230 User a@localhost:2222 logged in.
        ftp> cd ../../../tst/manyfiles
        250 CWD command successful.
        ftp> ls
        200 PORT command successful.
        150 Opening ASCII mode data connection for nlst.
        Connection closed by remote host.
        ftp> o localhost 2120
        Connected to WKE198515.u-dom1.u-ssi.net.
        220 FTPToSFTPProxy use 'ssh2-user@ssh2-host[:port]' as your username or ;ssh2-us
        er;keyfile;10@ssh2-host[:port]
        User (WKE198515.u-dom1.u-ssi.net:(none)): a@localhost:2222
        331 Password required for a@localhost:2222.
        Password:
        230 User a@localhost:2222 logged in.
        ftp> cd ../../../tst/manyfiles
        250 CWD command successful.
        ftp> ls
        200 PORT command successful.
        150 Opening ASCII mode data connection for nlst.
        Connection closed by remote host.
        ftp>

        mindterm is used in ftp2sftp mode
        trying to list files in a directory "manyfiles" with > 1500 files doesn't work on the "apache mina sshd"
        when doing the following operation it doesn't work. In the "manyfiles" dir there are >1500 files.

        mindterm can list files in a directory with < 50 files
        mindterm can list files on a opensshd server with a directory with > 1500 files, therefore we think the error is in the "apache mina sshd" and not in mindterm, at least there is some incompatibility...


        ftp> o localhost 2120
        Connected to WKE198515.u-dom1.u-ssi.net.
        220 FTPToSFTPProxy use 'ssh2-user@ssh2-host[:port]' as your username or ;ssh2-us
        er;keyfile;10@ssh2-host[:port]
        User (WKE198515.u-dom1.u-ssi.net:(none)): a@localhost:2222
        331 Password required for a@localhost:2222.
        Password:
        230 User a@localhost:2222 logged in.
        ftp> cd ../../../tst/manyfiles
        250 CWD command successful.
        ftp> ls
        200 PORT command successful.
        150 Opening ASCII mode data connection for nlst.
        Connection closed by remote host.
        Hide
        Otto Frost added a comment -

        The apache sshd SFTP daemon should be fixed so it keeps the SFTP packet size below the
        recommended 34000 bytes. The difference in behaviour between OpenSSH
        SFTP and MindTerm SFTP is due to the fact the OpenSSH allows a max SFTP
        packet size of 256KBytes while MindTerm SFTP has 34000 bytes.

        If you want to get the same behaviour with MindTerm as with OpenSSH, change
        com.mindbright.ssh2.SSH2SFTP:DATA_BUFFER_SIZE to 256*1024. But the correct
        fix is to make sure the SFTP daemon never sends larger packets than
        34000 bytes -
        you will still need to implement a limit since OpenSSH SFTP will refuse
        to accepts
        directory listings which results in packets > 256KBytes...

        The Apache SSH server is also broken in that it sends SSH_MSG_CHANNEL_EOF
        SSH_MSG_CHANNEL_REQUEST("exit-status") after SSH_MSG_CHANNEL_CLOSE has
        been sent.

        Show
        Otto Frost added a comment - The apache sshd SFTP daemon should be fixed so it keeps the SFTP packet size below the recommended 34000 bytes. The difference in behaviour between OpenSSH SFTP and MindTerm SFTP is due to the fact the OpenSSH allows a max SFTP packet size of 256KBytes while MindTerm SFTP has 34000 bytes. If you want to get the same behaviour with MindTerm as with OpenSSH, change com.mindbright.ssh2.SSH2SFTP:DATA_BUFFER_SIZE to 256*1024. But the correct fix is to make sure the SFTP daemon never sends larger packets than 34000 bytes - you will still need to implement a limit since OpenSSH SFTP will refuse to accepts directory listings which results in packets > 256KBytes... The Apache SSH server is also broken in that it sends SSH_MSG_CHANNEL_EOF SSH_MSG_CHANNEL_REQUEST("exit-status") after SSH_MSG_CHANNEL_CLOSE has been sent.
        Hide
        Bobby Powers added a comment -

        Yup. the relevent part of the spec (for reference) is:

        8.2.2. Reading Directories

        In order to retrieve a directory listing, the client issues one or
        more SSH_FXP_READDIR requests. In order to obtain a complete
        directory listing, the client MUST issue repeated SSH_FXP_READDIR
        requests until the server responds with an SSH_FXP_STATUS message.

        byte SSH_FXP_READDIR
        uint32 request-id
        string handle

        handle
        'handle' is a handle returned by SSH_FXP_OPENDIR. If 'handle' is
        an ordinary file handle returned by SSH_FXP_OPEN, the server MUST
        return SSH_FX_INVALID_HANDLE.

        The server responds to this request with either a SSH_FXP_NAME or a
        SSH_FXP_STATUS message. One or more names may be returned at a time.
        Full status information is returned for each name in order to speed
        up typical directory listings.

        If there are no more names available to be read, the server MUST
        respond with a SSH_FXP_STATUS message with error code of SSH_FX_EOF.

        sendName() never checks the size of the buffer its creating, so its not surprising it blows through the packet size limit for large dirs. Good detective work

        Show
        Bobby Powers added a comment - Yup. the relevent part of the spec (for reference) is: 8.2.2. Reading Directories In order to retrieve a directory listing, the client issues one or more SSH_FXP_READDIR requests. In order to obtain a complete directory listing, the client MUST issue repeated SSH_FXP_READDIR requests until the server responds with an SSH_FXP_STATUS message. byte SSH_FXP_READDIR uint32 request-id string handle handle 'handle' is a handle returned by SSH_FXP_OPENDIR. If 'handle' is an ordinary file handle returned by SSH_FXP_OPEN, the server MUST return SSH_FX_INVALID_HANDLE. The server responds to this request with either a SSH_FXP_NAME or a SSH_FXP_STATUS message. One or more names may be returned at a time. Full status information is returned for each name in order to speed up typical directory listings. If there are no more names available to be read, the server MUST respond with a SSH_FXP_STATUS message with error code of SSH_FX_EOF. sendName() never checks the size of the buffer its creating, so its not surprising it blows through the packet size limit for large dirs. Good detective work
        Hide
        Bobby Powers added a comment -

        can you open a separate bug for the SSH_MSG_CHANNEL_EOF issue with some more details?

        Show
        Bobby Powers added a comment - can you open a separate bug for the SSH_MSG_CHANNEL_EOF issue with some more details?
        Hide
        Otto Frost added a comment -

        Credits should go to the Mindterm developers at Cryptzone http://www.cryptzone.com/products/agmindterm/

        I have no more information on the SSH_MSG_CHANNEL_EOF issue currently.

        You can download mindterm for free if you like to replicate the issue.
        In the examples directory there is the class FTPToSFTPProxy.

        I'm using a modified version of mindterm, with pub/key authentication, and a fix for filename pattern matching (using regexp).

        To replicate the issue you should be fine with the out of the box mindterm though I think.

        I think I don't know the apache sshd well enough to implement a correction, hope you can provide a correction. Testing it I can do.

        Show
        Otto Frost added a comment - Credits should go to the Mindterm developers at Cryptzone http://www.cryptzone.com/products/agmindterm/ I have no more information on the SSH_MSG_CHANNEL_EOF issue currently. You can download mindterm for free if you like to replicate the issue. In the examples directory there is the class FTPToSFTPProxy. I'm using a modified version of mindterm, with pub/key authentication, and a fix for filename pattern matching (using regexp). To replicate the issue you should be fine with the out of the box mindterm though I think. I think I don't know the apache sshd well enough to implement a correction, hope you can provide a correction. Testing it I can do.
        Hide
        Otto Frost added a comment -

        Correction implemented, really ugly, but the job gets done

        in the SftpSubsystem class

        case SSH_FXP_READDIR: {
        System.out.println("SftpSybsystem.process SSH_FXP_READDIR");
        String handle = buffer.getString();
        try {
        Handle p = handles.get(handle);
        if (!(p instanceof DirectoryHandle))

        { sendStatus(id, SSH_FX_INVALID_HANDLE, handle); }

        else if (((DirectoryHandle) p).isDone())

        { sendStatus(id, SSH_FX_EOF, "", ""); }

        else if (!p.getFile().doesExist())

        { sendStatus(id, SSH_FX_NO_SUCH_FILE, p.getFile() .getAbsolutePath()); }

        else if (!p.getFile().isDirectory())

        { sendStatus(id, SSH_FX_NOT_A_DIRECTORY, p.getFile() .getAbsolutePath()); }

        else if (!p.getFile().isReadable())

        { sendStatus(id, SSH_FX_PERMISSION_DENIED, p.getFile() .getAbsolutePath()); }

        else {
        DirectoryHandle dh = (DirectoryHandle) p;
        List<SshFile> listSshFiles = dh.getFile().listSshFiles();

        SshFile[] virtualFiles = new SshFile[1];
        for (int i = 0; i < 1; ++i)

        { virtualFiles[i] = listSshFiles.get(dh.getCurrentFile()); dh.increaseCurrentFile(); }

        List<SshFile> listSshFiles2 = Collections.unmodifiableList(Arrays.asList(virtualFiles));

        //listSshFiles.get(index);
        //sendName(id, p.getFile().listSshFiles());
        sendName(id, listSshFiles2);

        if(dh.getCurrentFile()>= listSshFiles.size())

        { ((DirectoryHandle) p).setDone(true); //sendStatus(id,SSH_FX_EOF,"" ); }

        }
        } catch (IOException e)

        { sendStatus(id, SSH_FX_FAILURE, e.getMessage()); }

        break;
        }

        protected static class DirectoryHandle extends Handle {
        boolean done;
        int currentFile = 0;
        public DirectoryHandle(SshFile file)

        { super(file); }

        public boolean isDone()

        { return done; }

        public void setDone(boolean done)

        { this.done = done; }

        public int getCurrentFile()

        { return currentFile; }

        public void increaseCurrentFile()

        { currentFile++; }

        }

        Show
        Otto Frost added a comment - Correction implemented, really ugly, but the job gets done in the SftpSubsystem class case SSH_FXP_READDIR: { System.out.println("SftpSybsystem.process SSH_FXP_READDIR"); String handle = buffer.getString(); try { Handle p = handles.get(handle); if (!(p instanceof DirectoryHandle)) { sendStatus(id, SSH_FX_INVALID_HANDLE, handle); } else if (((DirectoryHandle) p).isDone()) { sendStatus(id, SSH_FX_EOF, "", ""); } else if (!p.getFile().doesExist()) { sendStatus(id, SSH_FX_NO_SUCH_FILE, p.getFile() .getAbsolutePath()); } else if (!p.getFile().isDirectory()) { sendStatus(id, SSH_FX_NOT_A_DIRECTORY, p.getFile() .getAbsolutePath()); } else if (!p.getFile().isReadable()) { sendStatus(id, SSH_FX_PERMISSION_DENIED, p.getFile() .getAbsolutePath()); } else { DirectoryHandle dh = (DirectoryHandle) p; List<SshFile> listSshFiles = dh.getFile().listSshFiles(); SshFile[] virtualFiles = new SshFile [1] ; for (int i = 0; i < 1; ++i) { virtualFiles[i] = listSshFiles.get(dh.getCurrentFile()); dh.increaseCurrentFile(); } List<SshFile> listSshFiles2 = Collections.unmodifiableList(Arrays.asList(virtualFiles)); //listSshFiles.get(index); //sendName(id, p.getFile().listSshFiles()); sendName(id, listSshFiles2); if(dh.getCurrentFile()>= listSshFiles.size()) { ((DirectoryHandle) p).setDone(true); //sendStatus(id,SSH_FX_EOF,"" ); } } } catch (IOException e) { sendStatus(id, SSH_FX_FAILURE, e.getMessage()); } break; } protected static class DirectoryHandle extends Handle { boolean done; int currentFile = 0; public DirectoryHandle(SshFile file) { super(file); } public boolean isDone() { return done; } public void setDone(boolean done) { this.done = done; } public int getCurrentFile() { return currentFile; } public void increaseCurrentFile() { currentFile++; } }
        Hide
        Otto Frost added a comment -

        bug fix, cas with empty dir not handled

        case SSH_FXP_READDIR: {
        // System.out.println("SftpSybsystem.process SSH_FXP_READDIR");
        String handle = buffer.getString();
        try {
        Handle p = handles.get(handle);
        if (!(p instanceof DirectoryHandle))

        { sendStatus(id, SSH_FX_INVALID_HANDLE, handle); }

        else if (((DirectoryHandle) p).isDone())

        { sendStatus(id, SSH_FX_EOF, "", ""); }

        else if (!p.getFile().doesExist())

        { sendStatus(id, SSH_FX_NO_SUCH_FILE, p.getFile() .getAbsolutePath()); }

        else if (!p.getFile().isDirectory())

        { sendStatus(id, SSH_FX_NOT_A_DIRECTORY, p.getFile() .getAbsolutePath()); }

        else if (!p.getFile().isReadable())

        { sendStatus(id, SSH_FX_PERMISSION_DENIED, p.getFile() .getAbsolutePath()); }

        else {
        DirectoryHandle dh = (DirectoryHandle) p;
        List<SshFile> listSshFiles = dh.getFile().listSshFiles();
        if(listSshFiles.size() == 0)

        { dh.setDone(true); sendStatus(id, SSH_FX_EOF, "", ""); }

        else{
        SshFile[] virtualFiles = new SshFile[1];
        for (int i = 0; i < 1; ++i)

        { virtualFiles[i] = listSshFiles.get(dh.getCurrentFile()); dh.increaseCurrentFile(); }

        List<SshFile> listSshFiles2 = Collections.unmodifiableList(Arrays.asList(virtualFiles));

        //listSshFiles.get(index);
        //sendName(id, p.getFile().listSshFiles());
        sendName(id, listSshFiles2);

        if(dh.getCurrentFile()>= listSshFiles.size())

        { ((DirectoryHandle) p).setDone(true); //sendStatus(id,SSH_FX_EOF,"" ); }

        }
        }
        } catch (IOException e)

        { sendStatus(id, SSH_FX_FAILURE, e.getMessage()); }

        break;
        }

        Show
        Otto Frost added a comment - bug fix, cas with empty dir not handled case SSH_FXP_READDIR: { // System.out.println("SftpSybsystem.process SSH_FXP_READDIR"); String handle = buffer.getString(); try { Handle p = handles.get(handle); if (!(p instanceof DirectoryHandle)) { sendStatus(id, SSH_FX_INVALID_HANDLE, handle); } else if (((DirectoryHandle) p).isDone()) { sendStatus(id, SSH_FX_EOF, "", ""); } else if (!p.getFile().doesExist()) { sendStatus(id, SSH_FX_NO_SUCH_FILE, p.getFile() .getAbsolutePath()); } else if (!p.getFile().isDirectory()) { sendStatus(id, SSH_FX_NOT_A_DIRECTORY, p.getFile() .getAbsolutePath()); } else if (!p.getFile().isReadable()) { sendStatus(id, SSH_FX_PERMISSION_DENIED, p.getFile() .getAbsolutePath()); } else { DirectoryHandle dh = (DirectoryHandle) p; List<SshFile> listSshFiles = dh.getFile().listSshFiles(); if(listSshFiles.size() == 0) { dh.setDone(true); sendStatus(id, SSH_FX_EOF, "", ""); } else{ SshFile[] virtualFiles = new SshFile [1] ; for (int i = 0; i < 1; ++i) { virtualFiles[i] = listSshFiles.get(dh.getCurrentFile()); dh.increaseCurrentFile(); } List<SshFile> listSshFiles2 = Collections.unmodifiableList(Arrays.asList(virtualFiles)); //listSshFiles.get(index); //sendName(id, p.getFile().listSshFiles()); sendName(id, listSshFiles2); if(dh.getCurrentFile()>= listSshFiles.size()) { ((DirectoryHandle) p).setDone(true); //sendStatus(id,SSH_FX_EOF,"" ); } } } } catch (IOException e) { sendStatus(id, SSH_FX_FAILURE, e.getMessage()); } break; }
        Hide
        Guillaume Nodet added a comment -

        Do you think you could provide a diff file ? It's way easier to review.

        Show
        Guillaume Nodet added a comment - Do you think you could provide a diff file ? It's way easier to review.
        Otto Frost made changes -
        Attachment sftpsubsystem.diff [ 12477099 ]
        Hide
        Otto Frost added a comment -

        diff attached

        I hardcoded the numer of files per packet to 5. It would be better to calculate the number of files in relation to the packet size.
        It causes more roundtrips than necessary, but it works.

        Show
        Otto Frost added a comment - diff attached I hardcoded the numer of files per packet to 5. It would be better to calculate the number of files in relation to the packet size. It causes more roundtrips than necessary, but it works.
        Hide
        Bobby Powers added a comment -

        Thanks for the diff!. Yea, I think it makes sense to try to fill the buffer up to (but not over) the packet size as you said. I'll take a stab at this this weekend, unless someone beats me to it.

        Show
        Bobby Powers added a comment - Thanks for the diff!. Yea, I think it makes sense to try to fill the buffer up to (but not over) the packet size as you said. I'll take a stab at this this weekend, unless someone beats me to it.
        Hide
        Guillaume Nodet added a comment -

        Thx for the patch Otto.
        I've reworked it a bit to minimize the number of packets send by filling the buffer up to a certain point instead of limiting the number of files sent.

        Show
        Guillaume Nodet added a comment - Thx for the patch Otto. I've reworked it a bit to minimize the number of packets send by filling the buffer up to a certain point instead of limiting the number of files sent.
        Guillaume Nodet made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Guillaume Nodet [ gnt ]
        Fix Version/s 0.7.0 [ 12317953 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Guillaume Nodet
            Reporter:
            Otto Frost
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 168h
              168h
              Remaining:
              Remaining Estimate - 168h
              168h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development