On 4 September 2011 16:43, Daniel Stenberg <daniel_at_haxx.se> wrote:
> 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?
git checkout 8ce9a66: works
git checkout 90b4b40: doesn't
git checkout master: doesn't
git checkout libssh2-1.2.9: doesn't
>>> ... 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).
Ah, I'm with you now. At the same time as fixing this, we may need to
add some more comments :P
>> 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 */
This patch fixes it for me :)
> On a second thought, that's too simple and will make a 'gap' in the file... I'll have to
> think a while more and provide a better patch soon.
You mean where later SFTP_READs were sent out requesting data from the
full chunk->len offset onwards? But now we need to make another
request for the range between (chunk->len - rc32) and chunk->len? Can
we just send out another SFTP_READ requesting this or do they have to
be sent sequentially?
Alex
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-09-04