Hi everyone, I took some time to dig into the SFTP upload issue with larger buffers, and I think I have identified the problem. Two problems, actually.
The first one is simple enough. The failure mode for this problem was that sometimes libssh2 would call select() but without waiting for any socket to become writable, and so it would hang forever. The cause is related to the same part of the code as the SCP issue (channel issue, really) that was solved earlier: _sftp_write() calls _channel_write() which calls _transport_write() which uses the send_existing() helper function to first send out any remaining data from a previous packet, in case that packet has not been fully sent yet. The previous problem was that send_existing() didn't return the correct status when a packet was still not sent completely. The problem now is that _transport_write() starts out with: /* default clear the bit */ session->socket_block_directions &= ~LIBSSH2_SESSION_BLOCK_OUTBOUND; This means that any subsequent call to select() will not watch for the socket to become writable. This might seem backwards in a write function. Anyway the flag is set again later in the function, once a packet has been fully prepared and bytes can actually be sent out. The problem comes, again, when the full packet is not sent on the two(!) first calls to _transport_write(). When increasing buffer sizes this becomes increasingly likely, although the exact details depends on the particular kernel's socket code. My 2.6 Linux takes 10-20k at a time from large buffers, when sending on loopback. Blocking mode is non-blocking internally in libssh2 and _transport_write() returns even when a packet is not yet completely sent. Eventually it is called again (in blocking mode this is done by libssh2) and then it will call send_existing() to try to finish the packet. However, with a large packet it may still not finish, and we go another round; return from _transport_write(), the libssh2 blocking emulation layer does select() to wait for the next opportunity to write .. except that it doesn't, since the directions flag was cleared on the previous call to _transport_write() (number two for this packet) and it never got set again! I solved this by moving the clearing of the write direction flag to after the send_existing() call has finished, so that the flag is only cleared once the existing partial packet has been sent completely, and _transport_write() is about to prepare a new packet. Commit 7317eda does this and I think it's an adequate solution, since send_existing() can't fail unless internal state has been messed up somehow, and then we'll lose anyway. However, this still doesn't completely fix the issue! The remaining problem is much more serious in a way, and has to do with how the API is very broken in a sense. You are encouraged to test the current git code, but please do not use buffer sizes that are exactly 32k or larger. In order to avoid the second issue described below you have to stay with buffers that are less than (32k - a few 100 bytes). For example, 32500 has been verified to work reliably here, but if you are using RANDOM_PADDING you may need to decrease your buffer size further, so that libssh2 has more room. 31k should always be safe. Daniel Stenberg wrote: >>> Oh to add to this, the corruption only happened when I was using a buffer >>> size larger than what libssh2's write packet size limit was (32*1024 I >>> believe was the limit) it would say it was splitting up the writes, I had >>> it set to 128*1024 for buffer size. Once I set it to 32*1024 or lower I >>> no longer had files corrupting when going to this OS X server. >> >> I think it could be an even more basic error because lots of code in >> libssh2 takes the passed in size, allocates that big of packet and sends >> that off. > > I've now made libssh2_channel_write_ex() deal with larger than 32K sizes by > simply ignoring everything beyond 32K. Since it returns the number of bytes > it sends it shouldn't cause any particular problems for existing apps. It turns out to be a big problem. Since each layer in SSH (SFTP, channel, transport) needs to add some bytes to every block of data passed in from upper layers, it is actually not correct for the upper layer (application, SFTP, channel) to use the size of data that it wants to send as reference for how many bytes need to be sent by the lower layer - since in fact there are extra bytes added by the lower layer, an increasing amount with more layers being involved. I don't have a patch to solve this yet. I think the ABI needs to break in order to fix this, so discussion would be good. A simple solution would be to not return bytes sent to the upper layer, but only LIBSSH2_ERROR_EAGAIN or 0 when done. Do you think that is sufficient? //Peter _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
