[prev in list] [next in list] [prev in thread] [next in thread] 

List:       proftpd-committers
Subject:    [ProFTPD-committers] CVS: proftpd/contrib/mod_sftp channel.c, 1.15,
From:       "TJ Saunders" <castaglia () users ! sourceforge ! net>
Date:       2009-08-24 2:16:54
Message-ID: E1MfP79-00080P-41 () ddv4jf1 ! ch3 ! sourceforge ! com
[Download RAW message or body]

Update of /cvsroot/proftp/proftpd/contrib/mod_sftp
In directory ddv4jf1.ch3.sourceforge.com:/tmp/cvs-serv30567/contrib/mod_sftp

Modified Files:
	channel.c channel.h scp.c 
Log Message:

Fix a rather egregious bug in the SCP download code.  We were reading the
entire file being downloaded into memory, and sending (or buffering) the
data for it BEFORE returning back to handling any messages which may have
been sent by the client while we were reading.  If the file is larger than
the default/initial window size, then we end up buffering.  We only drain
that buffered data once the file had been read in its entirety, and we
go back and handle any WINDOW_ADJUST messages, which then open the window
again and we can drain our data.

Now imagine if that file is large, say, >4GB.  The client sends its
WINDOW_ADJUST messages, but mod_sftp never gets around to listening to them
for quite a while.  Not good.  The fix, then, is to have the SCP download
code check the window size periodically.  If the window closes, then handle
enough messages from the client until the window opens again.


Index: channel.c
===================================================================
RCS file: /cvsroot/proftp/proftpd/contrib/mod_sftp/channel.c,v
retrieving revision 1.15
retrieving revision 1.16
diff -u -r1.15 -r1.16
--- channel.c	22 Aug 2009 02:57:38 -0000	1.15
+++ channel.c	24 Aug 2009 02:16:52 -0000	1.16
@@ -238,16 +238,11 @@
        * is the smaller of the remote window size and the remote packet size.
        */
 
-      if (payload_len > chan->remote_windowsz)
-        payload_len = chan->remote_windowsz;
-
       if (payload_len > chan->remote_max_packetsz)
         payload_len = chan->remote_max_packetsz;
 
-      if (payload_len == 0) {
-        /* Not enough room to send any data. */
-        break;
-      }
+      if (payload_len > chan->remote_windowsz)
+        payload_len = chan->remote_windowsz;
 
       pkt = sftp_ssh2_packet_create(tmp_pool);
 
@@ -268,7 +263,7 @@
       pkt->payload_len = (bufsz - buflen);
 
       pr_trace_msg(trace_channel, 9, "sending CHANNEL_DATA (remote channel "
-        "ID %lu, %lu bytes)", (unsigned long) chan->remote_channel_id,
+        "ID %lu, %lu data bytes)", (unsigned long) chan->remote_channel_id,
         (unsigned long) payload_len);
 
       res = sftp_ssh2_packet_write(sftp_conn->wfd, pkt);
@@ -282,6 +277,11 @@
 
       chan->remote_windowsz -= payload_len;
 
+      pr_trace_msg(trace_channel, 11,
+        "channel ID %lu remote window size currently at %lu bytes",
+        (unsigned long) chan->remote_channel_id,
+        (unsigned long) chan->remote_windowsz);
+
       /* If we sent this entire databuf, then we can dispose of it, and
        * advance to the next one on the list.  However, we may have only
        * sent a portion of it, in which case it needs to stay where it is;
@@ -1124,6 +1124,19 @@
   return 0;
 }
 
+uint32_t sftp_channel_get_windowsz(uint32_t channel_id) {
+  struct ssh2_channel *chan;
+
+  chan = get_channel(channel_id);
+  if (chan == NULL) {
+    pr_trace_msg(trace_channel, 1, "cannot return window size for unknown "
+      "channel ID %lu", (unsigned long) channel_id);
+    return 0;
+  }
+
+  return chan->remote_windowsz;
+}
+
 unsigned int sftp_channel_set_max_count(unsigned int max) {
   unsigned int prev_max;
 
@@ -1355,12 +1368,12 @@
      * is the smaller of the remote window size and the remote packet size.
      */ 
 
-    if (payload_len > chan->remote_windowsz)
-      payload_len = chan->remote_windowsz;
-
     if (payload_len > chan->remote_max_packetsz)
       payload_len = chan->remote_max_packetsz;
 
+    if (payload_len > chan->remote_windowsz)
+      payload_len = chan->remote_windowsz;
+
     if (payload_len > 0) {
       struct ssh2_packet *pkt;
       char *buf2, *ptr2;
@@ -1385,12 +1398,17 @@
       pkt->payload_len = (bufsz2 - buflen2);
 
       pr_trace_msg(trace_channel, 9, "sending CHANNEL_DATA (remote channel "
-        "ID %lu, %lu bytes)", (unsigned long) chan->remote_channel_id,
+        "ID %lu, %lu data bytes)", (unsigned long) chan->remote_channel_id,
         (unsigned long) payload_len);
 
       res = sftp_ssh2_packet_write(sftp_conn->wfd, pkt);
       if (res == 0) {
         chan->remote_windowsz -= payload_len;
+
+        pr_trace_msg(trace_channel, 11,
+          "channel ID %lu remote window size currently at %lu bytes",
+          (unsigned long) chan->remote_channel_id,
+          (unsigned long) chan->remote_windowsz);
       }
 
       /* If that was the entire payload, we can be done now. */
@@ -1428,7 +1446,7 @@
     memcpy(db->buf, buf, buflen);
 
     pr_trace_msg(trace_channel, 8, "buffering %lu remaining bytes of "
-      "outgoing packet", (unsigned long) buflen);
+      "outgoing data", (unsigned long) buflen);
   }
 
   return 0;

Index: channel.h
===================================================================
RCS file: /cvsroot/proftp/proftpd/contrib/mod_sftp/channel.h,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- channel.h	27 Jul 2009 01:00:59 -0000	1.4
+++ channel.h	24 Aug 2009 02:16:52 -0000	1.5
@@ -65,6 +65,7 @@
   int (*finish)(uint32_t);
 };
 
+uint32_t sftp_channel_get_windowsz(uint32_t);
 unsigned int sftp_channel_set_max_count(unsigned int);
 uint32_t sftp_channel_set_max_packetsz(uint32_t);
 uint32_t sftp_channel_set_max_windowsz(uint32_t);

Index: scp.c
===================================================================
RCS file: /cvsroot/proftp/proftpd/contrib/mod_sftp/scp.c,v
retrieving revision 1.23
retrieving revision 1.24
diff -u -r1.23 -r1.24
--- scp.c	27 Jul 2009 01:00:59 -0000	1.23
+++ scp.c	24 Aug 2009 02:16:52 -0000	1.24
@@ -1268,10 +1268,23 @@
     pr_config_get_xfer_bufsz()) + 1;
   chunk = palloc(p, chunksz);
 
-  /* Keep sending chunks until we have sent the entire file. */
+  /* Keep sending chunks until we have sent the entire file, or until the
+   * channel window closes.
+   */
   while (1) {
     pr_signals_handle();
 
+    /* If our channel window has closed, try handling some packets; hopefully
+     * some of them are WINDOW_ADJUST messages.
+     */
+    while (sftp_channel_get_windowsz(channel_id) == 0) {
+      pr_signals_handle();
+
+      if (sftp_ssh2_packet_handle() < 0) {
+        return 1;
+      }
+    }
+
     /* Seek to where we last left off with this file. */
     if (pr_fsio_lseek(sp->fh, sp->sentlen, SEEK_SET) < 0) {
       (void) pr_log_writefile(sftp_logfd, MOD_SFTP_VERSION,


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
ProFTPD Committers Mailing List
proftpd-committers@proftpd.org
https://lists.sourceforge.net/lists/listinfo/proftp-committers
[prev in list] [next in list] [prev in thread] [next in thread]