Re: [PATCH] gvfsd-ftp: fix codepath for receiving lines split in chunks.

[Andreas Henriksson <andreas fatal se>]

On Mon, Feb 16, 2009 at 07:57:53PM +0100, Andreas Henriksson wrote:
[...]
> I'm sending this mail to remind you of looking at my patch for bug 525283,
[...]
> Both attachments are also available (for a short time) on:
> http://fatal.se/tmp/gvfsd-ftp-chunked-lines.diff
> http://fatal.se/tmp/gvfsd-ftp-andreas.diff
[...]

As discussed on IRC.... the code may be in need of some refactoring.

I've attached my attempt. It's on top of the old patch.
Additionally I've also attached a compile warning fix...

Refactoring/warning patches also available here:
http://fatal.se/tmp/gvfsd-ftp-refactor-read-resp.diff
http://fatal.se/tmp/gvfsd-ftp-remove-connected.diff


After the refactoring the gvfs ftp backend can still connect to the problematic
site ftp://ftp.aist-nara.ac.jp/, but that's about all testing I've done.
(I don't have access to the original site anymore...)
I feel less confident about this, maybe it's better to postpone the
refactoring until after the next gnome release and just use the minimal fix
for now? Your choice...

-- 
Regards,
Andreas Henriksson

commit a656eab8a6661320751152c0cc1152c392844e4b
Author: Andreas Henriksson <andreas fatal se>
Date:   Mon Feb 16 22:23:16 2009 +0100

    gvfsd-ftp: refactor read response code.

diff --git a/daemon/gvfsbackendftp.c b/daemon/gvfsbackendftp.c
index 80a5c2f..7af46a1 100644
--- a/daemon/gvfsbackendftp.c
+++ b/daemon/gvfsbackendftp.c
@@ -350,7 +350,7 @@ ftp_connection_receive (FtpConnection *conn,
   SoupSocketIOStatus status;
   gsize n_bytes;
   gboolean got_boundary;
-  char *last_line, *last_chunk;
+  char *last_line;
   enum {
     FIRST_LINE,
     MULTILINE,
@@ -363,68 +363,65 @@ ftp_connection_receive (FtpConnection *conn,
   if (ftp_connection_in_error (conn))
     return 0;
 
-  got_boundary = TRUE;
+  /* read a full (multi-line?) ftp response */
   conn->read_bytes = 0;
   while (reply_state != DONE)
     {
-      if (conn->read_buffer_size - conn->read_bytes < 128)
-	{
-	  gsize new_size = conn->read_buffer_size + 1024;
-	  /* FIXME: upper limit for size? */
-	  gchar *new = g_try_realloc (conn->read_buffer, new_size);
-	  if (new)
-	    {
-	      conn->read_buffer = new;
-	      conn->read_buffer_size = new_size;
-	    }
-	  else
-	    {
-	      g_set_error_literal (&conn->error, G_IO_ERROR, G_IO_ERROR_FAILED,
-				   _("Invalid reply"));
-	      return 0;
-	    }
-	}
-      last_chunk = conn->read_buffer + conn->read_bytes;
-      if (got_boundary)
-        last_line = last_chunk;
-      status = soup_socket_read_until (conn->commands,
-				       last_chunk,
-				       /* -1 byte for nul-termination */
-				       conn->read_buffer_size - conn->read_bytes - 1,
-				       "\r\n",
-				       2,
-				       &n_bytes,
-				       &got_boundary,
-				       conn->job->cancellable,
-				       &conn->error);
-
-      conn->read_bytes += n_bytes;
-      conn->read_buffer[conn->read_bytes] = 0;
-      if (last_line == last_chunk)
-        DEBUG ("<-- %s", last_line);
-      else
-        DEBUG ("%s", last_chunk);
-
-      switch (status)
-	{
-	  case SOUP_SOCKET_OK:
-	  case SOUP_SOCKET_EOF:
-	    if (got_boundary)
-	      break;
-	    if (n_bytes > 0)
-	      continue;
-	    g_set_error_literal (&conn->error, G_IO_ERROR, G_IO_ERROR_FAILED,
-				 _("Invalid reply"));
-	    /* fall through */
-	  case SOUP_SOCKET_ERROR:
-	    conn->read_buffer[conn->read_bytes] = 0;
-	    return 0;
-	  case SOUP_SOCKET_WOULD_BLOCK:
-	  default:
-	    g_assert_not_reached ();
-	    break;
-	}
-
+      /* read a full line into the buffer */
+      last_line = conn->read_buffer + conn->read_bytes;
+      do {
+          char *last_chunk;
+
+          /* make sure we have free room in the buffer for more data */
+          if (conn->read_buffer_size - conn->read_bytes < 128)
+            {
+              gsize new_size = conn->read_buffer_size + 1024;
+              /* FIXME: upper limit for size? */
+              gchar *new = g_try_realloc (conn->read_buffer, new_size);
+              if (new)
+                {
+                  last_line = new + (last_line - conn->read_buffer);
+
+                  conn->read_buffer = new;
+                  conn->read_buffer_size = new_size;
+                }
+              else
+                {
+                  g_set_error_literal (&conn->error, G_IO_ERROR,
+                                       G_IO_ERROR_FAILED, _("Invalid reply"));
+                  return 0;
+                }
+            }
+
+
+	  /* read chunk of data into buffer */
+          last_chunk = conn->read_buffer + conn->read_bytes;
+          status = soup_socket_read_until (conn->commands,
+                                           last_chunk,
+                                           /* -1 byte for nul-termination */
+                                           conn->read_buffer_size - conn->read_bytes - 1,
+                                           "\r\n",
+                                           2,
+                                           &n_bytes,
+                                           &got_boundary,
+                                           conn->job->cancellable,
+                                           &conn->error);
+
+          conn->read_bytes += n_bytes;
+          conn->read_buffer[conn->read_bytes] = 0;
+
+          if (status != SOUP_SOCKET_OK && got_boundary == FALSE)
+            {
+              g_set_error_literal (&conn->error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                                   _("Invalid reply"));
+              return 0;
+            }
+      } while (got_boundary == FALSE);
+
+      /* we have received enough chunks to have a complete line. */
+      DEBUG ("<-- %s", last_line);
+
+      /* parse last line to see if we need to fetch more lines. */
       if (reply_state == FIRST_LINE)
 	{
 	  if (n_bytes < 4 ||
@@ -459,6 +456,7 @@ ftp_connection_receive (FtpConnection *conn,
 	}
     }
 
+  /* we have received a full ftp reply message.... */
   switch (STATUS_GROUP (response))
     {
       case 0:

commit b71b9193f2234c493b66dd796e709f822664b727
Author: Andreas Henriksson <andreas fatal se>
Date:   Mon Feb 16 22:25:07 2009 +0100

    gvfsd-ftp: fix warning / remove unused variable.

diff --git a/daemon/gvfsbackendftp.c b/daemon/gvfsbackendftp.c
index 7af46a1..88bed59 100644
--- a/daemon/gvfsbackendftp.c
+++ b/daemon/gvfsbackendftp.c
@@ -910,7 +910,6 @@ ftp_connection_ensure_data_connection_pasv (FtpConnection *conn)
 {
   guint ip1, ip2, ip3, ip4, port1, port2;
   const char *s;
-  gboolean connected;
   SoupAddress *addr;
   guint status;