Skip to content

Commit

Permalink
SFTP: don't buffer up download requests (PuTTY doesn't)
Browse files Browse the repository at this point in the history
  • Loading branch information
@terrafrost
terrafrost committed Dec 25, 2019
1 parent 1dc39be commit 333e2e4
Showing 1 changed file with 8 additions and 41 deletions.
49 changes: 8 additions & 41 deletions phpseclib/Net/SFTP.php
View file
Expand Up @@ -1701,13 +1701,6 @@ function _setstat_recursive($path, $attr, &$i)
}

$i++;

if ($i >= NET_SFTP_QUEUE_SIZE) {
if (!$this->_read_put_responses($i)) {
return false;
}
$i = 0;
}
}
}

Expand All @@ -1717,11 +1710,8 @@ function _setstat_recursive($path, $attr, &$i)

$i++;

if ($i >= NET_SFTP_QUEUE_SIZE) {
if (!$this->_read_put_responses($i)) {
return false;
}
$i = 0;
if (!$this->_read_put_responses($i)) {
return false;
}

return true;
Expand Down Expand Up @@ -2091,7 +2081,7 @@ function put($remote_file, $data, $mode = NET_SFTP_STRING, $start = -1, $local_s

$subtemp = $offset + $sent;
$packet = pack('Na*N3a*', strlen($handle), $handle, $subtemp / 4294967296, $subtemp, strlen($temp), $temp);
if (!$this->_send_sftp_packet(NET_SFTP_WRITE, $packet)) {
if (!$this->_send_sftp_packet(NET_SFTP_WRITE, $packet, $i)) {
if ($mode & NET_SFTP_LOCAL_FILE) {
fclose($fp);
}
Expand All @@ -2103,14 +2093,6 @@ function put($remote_file, $data, $mode = NET_SFTP_STRING, $start = -1, $local_s
}

$i++;

if ($i == NET_SFTP_QUEUE_SIZE) {
if (!$this->_read_put_responses($i)) {
$i = 0;
break;
}
$i = 0;
}
}

if (!$this->_read_put_responses($i)) {
Expand Down Expand Up @@ -2398,10 +2380,8 @@ function delete($path, $recursive = true)
if (!$recursive) {
return false;
}
$i = 0;
$result = $this->_delete_recursive($path, $i);
$this->_read_put_responses($i);
return $result;

return $this->_delete_recursive($path);
}

$this->_remove_from_stat_cache($path);
Expand All @@ -2419,11 +2399,8 @@ function delete($path, $recursive = true)
* @return bool
* @access private
*/
function _delete_recursive($path, &$i)
function _delete_recursive($path)
{
if (!$this->_read_put_responses($i)) {
return false;
}
$i = 0;
$entries = $this->_list($path, true);

Expand Down Expand Up @@ -2451,13 +2428,6 @@ function _delete_recursive($path, &$i)
$this->_remove_from_stat_cache($temp);

$i++;

if ($i >= NET_SFTP_QUEUE_SIZE) {
if (!$this->_read_put_responses($i)) {
return false;
}
$i = 0;
}
}
}

Expand All @@ -2468,11 +2438,8 @@ function _delete_recursive($path, &$i)

$i++;

if ($i >= NET_SFTP_QUEUE_SIZE) {
if (!$this->_read_put_responses($i)) {
return false;
}
$i = 0;
if (!$this->_read_put_responses($i)) {
return false;
}

return true;
Expand Down

1 comment on commit 333e2e4

@terrafrost
Copy link
Member Author

@terrafrost terrafrost commented on 333e2e4 Dec 26, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(#1425 is the inspiration for this change)

phpseclib implemented "parallel uploads" in 2010 in response to a post on the old (now defunct) support forum (topic_id=14936). "parallel downloads" were implemented in 2016 in the master branch in PR #945. The queue size was changed from 50 to 32 as a result of #1062 (also in 2016). The specific issue was in regards to downloading files and I made the change to make it consistent with what I observed PuTTY to be doing for downloads.

The thing is... PuTTY appears to be handling downloads and uploads differently whereas phpseclib is handling them the same (phpseclib is handling both as PuTTY handles downloads).

The idea of sending SSH_FXP_WRITE packets in batches of 50 came as a result of errors that would crop up in the files if > 50 packets were sent at a time. Quoting one of the responses to the original support forum ticket:

SFTP with interleaved mode and limit fix to 100 : 27 to 31 seconds, 10% errors (on 20 copy)
SFTP with interleaved mode and limit fix to 50 : 26 to 28 seconds, no errors
SFTP with interleaved mode and limit fix to 10 : 29 to 36 seconds, no errors

I am a little worried that removing this limit will re-introduce errors but (1) PuTTY does it without issue and (2) last time the request identifier was always set to 0 whereas this time (as of #1343) it's not so maybe that'll make a difference.

As for my comments on PuTTY's behavior... this can be observed by using psftp to download and upload a 1MB file. This can be done by first running the following command on the CLI:

./psftp -pw pass -sshlog test.log user@website.tld

From there you can do one of these commands:

put \local\path\1mb 1mb
get 1mb \local\path\1mb

A cursory examination of PuTTY's source code seems to confirm this behavior.

Downloads

psftp.c does while (!xfer_done(xfer)) and xfer_download_queue(xfer):

https://github.com/github/putty/blob/49fb598b0e78d09d6a2a42679ee0649df482090e/psftp.c#L428

xfer_download_queue does while (xfer->req_totalsize < xfer->req_maxsize && !xfer->eof && !xfer->err):

https://github.com/github/putty/blob/49fb598b0e78d09d6a2a42679ee0649df482090e/sftp.c#L1158

xfer->req_totalsize is incremented with xfer->req_totalsize += rr->len and rr->len, by default, is 32768. xfer->req_maxsize is 1048576, as defined here:

https://github.com/github/putty/blob/49fb598b0e78d09d6a2a42679ee0649df482090e/sftp.c#L1133

1048576 / 32768 is 32, so phpseclib's queue'ing up 32 read requests at a time is consistent with PuTTY's behavior.

Uploads

psftp.c does repeated calls to xfer_upload_data(xfer, buffer, len) while xfer_upload_ready(xfer). When xfer_upload_data(xfer, buffer, len) returns false it then receives the responses vis-a-vis xfer_upload_gotpkt(xfer, pktin):

https://github.com/github/putty/blob/49fb598b0e78d09d6a2a42679ee0649df482090e/psftp.c#L675

Here's xfer_upload_ready:

int xfer_upload_ready(struct fxp_xfer *xfer)
{
    if (sftp_sendbuffer() == 0)
	return 1;
    else
	return 0;
}

sftp.h says the following of sftp_sendbuffer:

 * sftp_sendbuffer returns the size of the backlog of data in the
 * transmit queue.

https://github.com/github/putty/blob/49fb598b0e78d09d6a2a42679ee0649df482090e/sftp.h#L58

It's not actually clear to me how sftp_sendbuffer works but I'm kinda wondering if it may not be applicable for PHP. phpseclib uses fputs in send_binary_packet. fputs is an alias of fwrite, which "returns the number of bytes written". If the number of bytes written is not the length of the string send_binary_packet returns false. So idk.

Please sign in to comment.