On Sat, 27 Feb 2010, Peter Stuge wrote:
>> The API for libssh2_sftp_write says that if the return value is
>> positive, it is the number of bytes actually written.
>
> Your understanding of the API is correct, but the implementation is only
> correct if you try to write less than 32768 - n bytes at a time, where n is
> the bytes needed by libssh2 internally to create the actual packets that
> will be sent to the kernel.
What's making that restriction? I'm quite sure I've done larger chunks than so
in the past. (Which made me introduce the count > (MAX_SSH_PACKET_LEN*4))
logic...)
> This is indeed a serious bug, and there doesn't seem to be a simple
> fix for it.
Sorry, can you elaborate on what this problem is in the code? Why would
libssh2 have any problem with large buffer sizes (or small) that's very hard
to fix?
> The current recommendation for a workaround is to never give libssh2 more
> than 32500 bytes to send out, through any function. Then the implementation
> should not exhibit any problem.
Right, but the fix is rather lame since we could easily just internally use
the maximum limit we support and thus not expose the inability.
>> I think the problem is in the following part of sftp.c
> ..
>> My packet_len is 32768+handle_len(4)+25 = 32797, and
>> _libssh2_channel_write can only write 32768 bytes at a time,
>
> The transport layer (ie. _libssh2_channel_write) also suffers from
> the deficiency outlined above.
_libssh2_channel_write does not NEED to ever send anything more than 32768
bytes at a time, so I disagree with this conclusion.
If that limitation causes a problem, the problem is in the surrounding code
and not in the _libssh2_channel_write() function itself.
A different approach could possibly be to allow _libssh2_channel_write() to
make multiple outgoing SSH transport packets when > 32768 bytes are passed to
it and send them. Hm, that might in fact be a more attractive way to proceed.
>> so rc is 32768 (the actual amount of the packet written so far).
>
> This isn't true unfortunately. The logic in _libssh2_channel_write simply
> fails when more data is coming in from the application than there is room
> for in buffers within the function. The fact that _write() doesn't have the
> same type of API as system write() (return number of bytes consumed) further
> complicates this for the implementation.
In what way doesn't it have that API? write() returns the number of payload
data it sent, it does not tell anything about how much protocol metadata it
dealt with. IMHO, that's what _libssh2_channel_write() does too. Doesn't it?
> There is a disconnect in API and in implementation between number of bytes
> submitted by application, and number of bytes sent to operating system on
> the socket.
But no ordinary application will have any idea how many bytes a message of N
bytes will end up being sent to the socket, so I don't see how the number of
bytes on the socket level is what an app wants to know.
> One idea to resolve this situation is described in
> http://libssh2.stuge.se/ticket/161
Right, but that's a rather drastic rewrite of large parts of the library so
it's not a small nor quick job.
> I also want to fix the current implementation so that the library actually
> works correctly regardless of implementation details.
Me too. I'm convinced there's no major flaws in the way the API is designed
that prevents it from working fine for SCP and SFTP transfers. I've played too
little with other aspects of the SSH stuff to tell for sure about those. I'm
aware we lack some SFTP stuff to make "perfect" SFTP transfers and I'll
present a suggestion on how to improve that sometime in the future.
> I believe that passing 32768 bytes into libssh2 will derail the SSH protocol
> communication and in effect make the library lock up at some point. (In
> non-blocking mode it would just return EAGAIN ad infitium.)
I don't understand why it would end up in that situation. Can you elaborate?
-- / daniel.haxx.se _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-develReceived on 2010-02-28