-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File is getting corrupted when Chunk Mode file transfer interrupted #315
base: master
Are you sure you want to change the base?
Conversation
Hi @anamikagsingh,
Thanks, |
Hi Jeremy, Could you please let me know if I need to format the file and commit once again or it can be done on committed file too? In our application, we have an option to transfer large files in smaller chunks and append them in the target in the sequence they have been transferred. But in case the transfer of chunk to the target directory is interrupted due to any issues (i.e., network failure, engine restart etc.), it will transfer the complete chunk again and as a result, the target file gets corrupted. To solve this, chunk transfer should be resumed instead of transferring the complete chunk again. And, to achieve this, we added a method checkChunkFailed() in ChannelSftp class which checks the bytes which is already transferred and accordingly set the information whether the chunk transfer is interrupted or not. Also, in SFTP application, we have used the below method which will try to resume the chunk transfer if transfer of chunk to the target directory is interrupted.
public void setChunkFailed(boolean chunk_failed, String chunk_start){ |
Hi Jeremy, I also have added changed files 'Session.java' and 'UserAuthKeyboardInteractive.java' to this pull request. This change is to fix the issue raised in #302 and also to support multiple server fingerprint. Regards, |
Hi @anamikagsingh,
Thanks, |
@@ -129,7 +129,8 @@ public boolean start(Session session) throws Exception { | |||
byte[][] response = null; | |||
|
|||
if (password != null && prompt.length == 1 && !echo[0] | |||
&& prompt[0].toLowerCase().indexOf("password:") >= 0) { | |||
&& (prompt[0].toLowerCase().indexOf("password:") >= 0 | |||
|| prompt[0].toLowerCase().indexOf("password") >= 0)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #302 you mention that this change is needed in order to support a server that has a prompt of Enter password:
.
However, the string Enter password:
already should match prompt[0].toLowerCase().indexOf("password:") >= 0
, so I'm not sure I understand the purpose of this change (see below)?
jshell> String foo = "Enter password:";
foo ==> "Enter password:"
jshell> if (foo.toLowerCase().indexOf("password:") >= 0) System.out.println("matches"); else throw new Exception("fail");
matches
jshell>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to reproduce this issue in my test system so cannot test it. We can leave the changes done in UserAuthKeyboardInteractive.java for now. Please let me know if I need to revert the changes and commit again. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are not able to reproduce the issue that you believed was fixed by this change, then it should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The or also overlaps, you could just remove the colon..
@@ -643,6 +653,32 @@ public void _put(InputStream src, String dst, SftpProgressMonitor monitor, int m | |||
throw (SftpException) e; | |||
throw new SftpException(SSH_FX_FAILURE, e.toString(), e); | |||
} | |||
|
|||
public boolean checkChunkFailed(InputStream src, String dst, String ChunkStart)throws SftpException{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The argument
InputStream src
appears to be unused in this method, so why is it needed? - The argument
String ChunkStart
should be changed to not start with a capital letter.
-- Perhaps something likeString chunkStart
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Removed the argument InputStream src from the method.
- Changed the parameter name to startChunk
@@ -32,6 +32,9 @@ | |||
import java.util.Hashtable; | |||
import java.util.Vector; | |||
|
|||
import com.jcraft.jsch.Channel.MyPipedInputStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary.
Since ChannelSftp
extends Channel
, it can directly reference the MyPipedInputStream
class without an import statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the import as it was not needed. Thanks for the suggestions.
throw new SftpException(SSH_FX_FAILURE, "failed to resume for "+dst); | ||
} | ||
} | ||
else if(mode==RESUME && skip>0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be better written as simply something like this to avoid code duplication:
if (mode == RESUME && skip > 0) {
long srcSkip = chunkStart != null ? skip - Long.parseLong(chunkStart) : skip;
long skipped = src.skip(srcSkip);
if (skipped < srcSkip) {
throw new SftpException(SSH_FX_FAILURE, "failed to resume for " + dst);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the changes as suggested.
@@ -158,6 +161,7 @@ public class ChannelSftp extends ChannelSession { | |||
private boolean fEncoding_is_utf8 = true; | |||
|
|||
private RequestQueue rq = new RequestQueue(16); | |||
private String chunkStart = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does chunkStart
need to be a String
instead of having it simply be a long
?
This would avoid a lot of unwieldy calls to Long.parseLong(chunkStart)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to Long now and made the other changes accordingly.
break; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you point at any other applications or libraries that perform SSH host key verification using fingerprints?
This seems like a pretty unusual (and potentially insecure) practice that I am unaware of, and I'm not sure we want to directly enable this sort of thing natively within JSch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea if any other application which is using Jsch library is also using fingerprints to perform SSH host key verification. It is being used in SAP PI SFTP Adapter.
@@ -129,6 +129,8 @@ public class Session { | |||
|
|||
Buffer buf; | |||
Packet packet; | |||
|
|||
List<String> fingerprintList = new ArrayList<String>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be shortened to simply new ArrayList<>()
instead of new ArrayList<String>()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it as suggested.
HI @anamikagsingh, Can you please answer the following questions?
Thanks, |
About point 1, when chunkmode is enabled, we set the chunk size (i.e. 10 MB). Based on chunk size, the bytes will be read from the input file and processed further. We are using the below snippet for chunk mode transfer in Adapter:
About point 2, I will try to figure it out. Thanks. |
Hi @anamikagsingh, From what I can understand, this Thanks, |
|
Hi,
This change will fix the Chunk mode issue raised in #308
Regards,
Anamika