On Sun, 4 Sep 2011, Alexander Lamaison wrote:
> Yes. Just double-checked: 90b4b40 exhibits the problem. If I revert to its
> predecessor, 8ce9a66, it works fine.
Do you revert to 8ce9a66, or do you run the latest with just 90b4b40 reverted?
Oh, and BTW, do you see this problem with a release version or with git head?
>> ... before this change, the 'count' variable could get wrapped in the second
>> call (as 'already' was then larger than buffer_size*4) and since count is
>> unsigned, it would turn into an insanly high value and just be very very
>> wrong.
>
> I see how the overflow could occur but I'm not sure the fix is right. What
> happens when (buffer_size)*4 is less that already? count becomes zero.
> This causes the whole packet sending loop to be skipped.
Yes, but that's on purpose. 'count' is how much more data to ask for, and
'already' is how much data that already has been asked for but not yet
returned. So, if 'already' is very large it should be perfectly fine to have
count set to 0 as then we don't have to ask for more data (right now).
> Maybe this is right (still trying to work out what the code is aiming to do)
> but it ends up causing (rc32 != chunk->len) to be true which prematurely
> signals the end of the file.
chunk->len is the size of data to read in a read packet, so when the
SSH_FXP_DATA response packet comes for that read it has to be of the required
size as only the last size is allowed to be shorter than requested...
Oh no!
I wonder if this isn't the problem. You mentioned how the server reads a file
that sometimes doesn't have data enough, so then it delivers a short packet
there and libssh2 then treats that short packet as end-of-file.
I now think this is a wrong assumption to do in libssh2 and I reread
draft-ietf-secsh-filexfer/draft-ietf-secsh-filexfer-04.txt now and I see no
guarantee for this. The server is allowed to return a short packet without it
meaning the end of the file!
This should be fixed. I'm thinking perhaps something like this:
diff --git a/src/sftp.c b/src/sftp.c
index 297dfa4..fa92f48 100644
--- a/src/sftp.c
+++ b/src/sftp.c
@@ -1242,9 +1242,12 @@ static ssize_t sftp_read(LIBSSH2_SFTP_HANDLE * handle,
cha
return _libssh2_error(session, LIBSSH2_ERROR_SFTP_PROTOCOL,
"SFTP Protocol badness");
- if(rc32 != chunk->len)
- /* a short read means this is the last read in the file */
- filep->eof = TRUE;
+ if(rc32 != chunk->len) {
+ /* a short read does not imply end of file, but we must adjust
+ the offset_sent since it was advanced with a full
+ chunk->len before */
+ filep->offset_sent -= (chunk->len - rc32);
+ }
if(total_read + rc32 > buffer_size) {
/* figure out the overlap amount */
-- / daniel.haxx.se _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-develReceived on 2011-09-04