GMail problems (was Re: ANNOUNCE: balsa-2.3.24 released)

[Pawel Salek <pawsa theochem kth se>]

On 05/31/2008 04:52:19 PM, Bruno Miguel wrote:
> I will stay tunned. I really like Balsa. :)

I think I have identified the remaining problems. Two of them.

1. Gmail sends data in a permuted order. When we ask for (BODY[HEADER]  
BODY[TEXT]), Gmail sends (BODY[TEXT] BODY[HEADER]) which is formally  
correct. In practice, balsa's ideas about message was incorrect (balsa  
looked for the headers and found none). The fix here is  
straightforward, we just need to look closer at the sent data.

2. Gmail does not follow the RFC3501 when generating BODYSTRUCTURE  
response. This response has "number-of-lines" field that is supposed to  
be present only for messages and text parts. Gmail sends it also for  
other parts, for example for application/pgp-signature. This obviously  
confuses the parser... I have implemented a workaround for this but the  
right solution is to fix Gmail IMAP server.

Anyway, attached patch fixes both of the problems for me. If I get  
reports that it works as expected, I will commit it and release 2.3.25.  
Anyway, somebody should contact Gmail regarding point 2...

Pawel


> On Sat, May 31, 2008 at 3:47 PM, Pawel Salek <pawsa theochem kth se>  
> wrote:
> > On 05/31/2008 04:46:19 PM, Pawel Salek wrote:
> >> On 05/31/2008 03:33:41 PM, Bruno Miguel wrote:
> >> > Good news. :)
> >> > Is Balsa's new version correctly fetching large number of emails
> >> from
> >> > gmail, using IMAP? (I've reported this bug about a month ago.)
> >>
> >> Partially... [*]
> >>
> >> I was about to write "yes, absolutely!" but I have just discovered
> >> that
> >> there is one more problem. Argh. GMIME sends the response in an
> >                                    ^^^^^ GMAIL.
> > _______________________________________________
> > balsa-list mailing list
> > balsa-list gnome org
> > http://mail.gnome.org/mailman/listinfo/balsa-list
> >
> _______________________________________________
> balsa-list mailing list
> balsa-list gnome org
> http://mail.gnome.org/mailman/listinfo/balsa-list

Index: libbalsa/imap/imap-handle.c
===================================================================
--- libbalsa/imap/imap-handle.c	(revision 7926)
+++ libbalsa/imap/imap-handle.c	(working copy)
@@ -2059,7 +2059,6 @@
     if(c=='~') /* BINARY extension literal8 indicator */
       c = sio_getc(sio);
     if(c!='{') {
-      g_string_free(res, TRUE);
       return NULL; /* ERROR */
     }
 
@@ -2083,7 +2082,7 @@
 imap_get_string(struct siobuf* sio)
 {
   GString * s = imap_get_string_with_lookahead(sio, sio_getc(sio));
-  return g_string_free(s, FALSE);
+  return s ? g_string_free(s, FALSE) : NULL;
 }
 
 static gboolean
@@ -2101,7 +2100,10 @@
   if(toupper(c)=='N') { /* nil */
     sio_getc(sio); sio_getc(sio); /* ignore i and l */
     return NULL;
-  } else return g_string_free(imap_get_string_with_lookahead(sio, c), FALSE);
+  } else {
+    GString *s = imap_get_string_with_lookahead(sio, c);
+    return s ? g_string_free(s, FALSE) : NULL;
+  }
 }
 
 /* see the spec for the definition of astring */
@@ -2977,7 +2979,7 @@
 {
   gchar *str = imap_get_nstring(h->sio);
   if(str && h->body_cb)
-    h->body_cb(seqno, str, strlen(str), h->body_arg);
+    h->body_cb(seqno, IMAP_BODY_TYPE_RFC822, str, strlen(str), h->body_arg);
   g_free(str);
   return IMR_OK;
 }
@@ -3391,7 +3393,23 @@
 
   if (type == IMB_NON_EXTENSIBLE)
     return IMR_PROTOCOL;
-
+#define GMAIL_BUG_20080601
+#ifdef GMAIL_BUG_20080601
+  /* GMail sends number of lines on some parts like application/pgp-signature */
+  { int c = sio_getc(sio);
+    if(c == -1)
+      return IMR_PROTOCOL;
+    sio_ungetc(sio);
+    if(isdigit(c)) { 
+      char buf[20];
+      printf("Incorrect GMail number-of-lines entry detected. "
+	     "Working around.\n");
+      c = imap_get_atom(sio, buf, sizeof(buf));
+      if(c != ' ')
+	return IMR_PROTOCOL;
+    }
+  }
+#endif
   /* body_fld_md5 = nstring */
   str = imap_get_nstring (sio);
   if (body && str)
@@ -3619,18 +3637,24 @@
 /* read [section] and following string. FIXME: other kinds of body. */ 
 static ImapResponse
 ir_body_section(struct siobuf *sio, unsigned seqno,
-		ImapFetchBodyCb body_cb, void *arg)
+		ImapFetchBodyType body_type,
+		ImapFetchBodyInternalCb body_cb, void *arg)
 {
   char buf[80];
   GString *bs;
-  int c = imap_get_atom(sio, buf, sizeof(buf));
+  int i, c = imap_get_atom(sio, buf, sizeof(buf));
 
+  for(i=0; buf[i] && (isdigit((int)buf[i]) || buf[i] == '.'); i++)
+    ;
+  if(i>0 && isalpha(buf[i])) /* we have \[[.0-9]something] */
+    body_type = IMAP_BODY_TYPE_HEADER;
+
   if(c != ']') { puts("] expected"); return IMR_PROTOCOL; }
   if(sio_getc(sio) != ' ') { puts("space expected"); return IMR_PROTOCOL;}
   bs = imap_get_binary_string(sio);
   if(bs) {
     if(bs->str && body_cb)
-      body_cb(seqno, bs->str, bs->len, arg);
+      body_cb(seqno, body_type, bs->str, bs->len, arg);
     g_string_free(bs, TRUE);
   }
   return IMR_OK;
@@ -3657,7 +3681,8 @@
 
   tmp = imap_get_nstring(h->sio);
   if(h->body_cb) {
-    if(tmp) h->body_cb(seqno, tmp, strlen(tmp), h->body_arg);
+    if(tmp) h->body_cb(seqno, IMAP_BODY_TYPE_HEADER,
+		       tmp, strlen(tmp), h->body_arg);
     g_free(tmp);
   } else {
     CREATE_IMSG_IF_NEEDED(h, seqno);
@@ -3680,15 +3705,19 @@
     c = sio_getc (h->sio);
     sio_ungetc (h->sio);
     if(isdigit (c)) {
-      rc = ir_body_section(h->sio, seqno, h->body_cb, h->body_arg);
+      rc = ir_body_section(h->sio, seqno, IMAP_BODY_TYPE_BODY,
+			   h->body_cb, h->body_arg);
       break;
     }
     c = imap_get_atom(h->sio, buf, sizeof buf);
     if (c == ']' &&
         (g_ascii_strcasecmp(buf, "HEADER") == 0 ||
          g_ascii_strcasecmp(buf, "TEXT") == 0)) {
+      ImapFetchBodyType body_type = 
+	(g_ascii_strcasecmp(buf, "TEXT") == 0)
+	? IMAP_BODY_TYPE_TEXT : IMAP_BODY_TYPE_HEADER;
       sio_ungetc (h->sio); /* put the ']' back */
-      rc = ir_body_section(h->sio, seqno, h->body_cb, h->body_arg);
+      rc = ir_body_section(h->sio, seqno, body_type, h->body_cb, h->body_arg);
     } else {
       if (c == ' ' && 
           (g_ascii_strcasecmp(buf, "HEADER.FIELDS") == 0 ||
Index: libbalsa/imap/imap_private.h
===================================================================
--- libbalsa/imap/imap_private.h	(revision 7926)
+++ libbalsa/imap/imap_private.h	(working copy)
@@ -35,6 +35,20 @@
 
 #include "imap-commands.h"
 
+typedef enum {
+  IMAP_BODY_TYPE_RFC822, /**< as fetched with RFC822 */
+  IMAP_BODY_TYPE_HEADER, /** header of the message: BODY[HEADER] */
+  IMAP_BODY_TYPE_TEXT,   /**< content of the message part: BODY[TEXT] */
+  IMAP_BODY_TYPE_BODY    /**< a part as fetched with BODY[x] */
+} ImapFetchBodyType;
+
+
+typedef void (*ImapFetchBodyInternalCb)(unsigned seqno,
+					ImapFetchBodyType body_type,
+					const char *buf,
+					size_t buflen, void* arg);
+
+
 struct _MboxView {
   unsigned *arr;
   unsigned allocated, entries;
@@ -93,7 +107,7 @@
 
   ImapFlagsCb flags_cb;
   void *flags_arg;
-  ImapFetchBodyCb body_cb;
+  ImapFetchBodyInternalCb body_cb;
   void *body_arg;
 
   ImapMonitorCb monitor_cb;
Index: libbalsa/imap/imap-commands.c
===================================================================
--- libbalsa/imap/imap-commands.c	(revision 7926)
+++ libbalsa/imap/imap-commands.c	(working copy)
@@ -1030,12 +1030,29 @@
 }
 
 static void
-write_nstring(unsigned seqno, const char *str, size_t len, void *fl)
+write_nstring(unsigned seqno, ImapFetchBodyType body_type,
+              const char *str, size_t len, void *fl)
 {
   if (fwrite(str, 1, len, (FILE*)fl) != len)
     perror("write_nstring");
 }
 
+struct FetchBodyPassthroughData {
+  ImapFetchBodyCb cb;
+  void *arg;
+};
+
+static void
+fetch_body_passthrough(unsigned seqno,
+		       ImapFetchBodyType body_type,
+		       const char *buf,
+		       size_t buflen, void* arg)
+{
+  struct FetchBodyPassthroughData* data = (struct FetchBodyPassthroughData*)arg;
+
+  data->cb(seqno, buf, buflen, data->arg);
+}
+
 ImapResponse
 imap_mbox_handle_fetch_rfc822(ImapMboxHandle* handle,
 			      unsigned cnt, unsigned *set,
@@ -1051,11 +1068,14 @@
   seq = imap_coalesce_set(cnt, set);
 
   if(seq) {
-    ImapFetchBodyCb cb = handle->body_cb;
-    void          *arg = handle->body_arg;
+    ImapFetchBodyInternalCb cb = handle->body_cb;
+    void                   *arg = handle->body_arg;
     gchar *cmd = g_strdup_printf("FETCH %s RFC822", seq);
-    handle->body_cb  = fetch_cb;
-    handle->body_arg = fetch_cb_data;
+    struct FetchBodyPassthroughData passthrough_data;
+    passthrough_data.cb = fetch_cb;
+    passthrough_data.arg = fetch_cb_data;
+    handle->body_cb  = fetch_cb ? fetch_body_passthrough : NULL;
+    handle->body_arg = &passthrough_data;
     rc = imap_cmd_exec(handle, cmd);
     handle->body_cb  = cb;
     handle->body_arg = arg;
@@ -1067,23 +1087,128 @@
   return rc;
 }
 
+/* When peeking into message, we need to assure that we save header
+   first and body later: the order the fields are returned is in
+   principle undefined. */
+struct FetchBodyHeaderText {
+  FILE *out_file;
+  char *body;
+  size_t length;
+  gboolean wrote_header;
+};
+
+static void
+write_header_text_ordered(unsigned seqno, ImapFetchBodyType body_type,
+			  const char *str, size_t len, void *arg)
+{
+  struct FetchBodyHeaderText *fbht = (struct FetchBodyHeaderText*)arg;
+  switch(body_type) {
+  case IMAP_BODY_TYPE_RFC822:
+    g_warning("Server sends unrequested RFC822 response");
+    break; /* This is really unexpected response! */
+  case IMAP_BODY_TYPE_HEADER:
+    if (fwrite(str, 1, len, fbht->out_file) != len)
+      perror("write_nstring");
+    fbht->wrote_header = TRUE;
+    if(fbht->body) {
+      if (fwrite(fbht->body, 1, fbht->length, fbht->out_file) != fbht->length)
+	perror("write_nstring");
+      g_free(fbht->body); fbht->body = NULL;
+    }
+    break;
+  case IMAP_BODY_TYPE_TEXT:
+  case IMAP_BODY_TYPE_BODY:
+    if(fbht->wrote_header) {
+      if (fwrite(str, 1, len, fbht->out_file) != len)
+	perror("write_nstring");
+    } else {
+      fbht->body = g_malloc(len);
+      fbht->length = len;
+      memcpy(fbht->body, str, len);
+    }
+    break;
+  }
+}
+
+/* There is some point to code reuse here. */
+struct PassHeaderTextOrdered {
+  ImapFetchBodyCb cb;
+  void *arg;
+  char *body;
+  size_t length;
+  gboolean wrote_header;
+};
+
+static void
+pass_header_text_ordered(unsigned seqno, ImapFetchBodyType body_type,
+			  const char *str, size_t len, void *arg)
+{
+  struct PassHeaderTextOrdered *phto = (struct PassHeaderTextOrdered*)arg;
+  switch(body_type) {
+  case IMAP_BODY_TYPE_RFC822:
+    g_warning("Server sends unrequested RFC822 response");
+    break; /* This is really unexpected response! */
+  case IMAP_BODY_TYPE_HEADER:
+    phto->cb(seqno, str, len, phto->arg);
+    phto->wrote_header = TRUE;
+    if(phto->body) {
+    printf("Writing saved body\n");
+      phto->cb(seqno, phto->body, phto->length, phto->arg);
+      g_free(phto->body); phto->body = NULL;
+    }
+    break;
+  case IMAP_BODY_TYPE_TEXT:
+  case IMAP_BODY_TYPE_BODY:
+    if(phto->wrote_header) {
+      phto->cb(seqno, str, len, phto->arg);
+    } else {
+      printf("saving body\n");
+      phto->body = g_malloc(len);
+      phto->length = len;
+      memcpy(phto->body, str, len);
+    }
+    break;
+  }
+}
+
+
 ImapResponse
 imap_mbox_handle_fetch_rfc822_uid(ImapMboxHandle* handle, unsigned uid, 
                                   gboolean peek, FILE *fl)
 {
   char cmd[80];
-  ImapFetchBodyCb cb = handle->body_cb;
+  ImapFetchBodyInternalCb cb = handle->body_cb;
   void          *arg = handle->body_arg;
   ImapResponse rc;
+  char *cmdstr;
+  struct FetchBodyHeaderText separate_arg;
 
+
   IMAP_REQUIRED_STATE1(handle, IMHS_SELECTED, IMR_BAD);
   HANDLE_LOCK(handle);
-  handle->body_cb  = write_nstring;
-  handle->body_arg = fl;
-  sprintf(cmd, peek 
-          ? "UID FETCH %u (BODY.PEEK[HEADER] BODY.PEEK[TEXT])"
-          : "UID FETCH %u RFC822", uid);
+
+  /* Consider switching between BODY.PEEK[HEADER] BODY.PEEK[TEXT] and
+     BODY[HEADER] BODY[TEXT] - this would simplify the callback
+     code. */
+  if(peek) {
+    handle->body_cb  = write_header_text_ordered;
+    handle->body_arg = &separate_arg;
+    separate_arg.body = NULL;
+    separate_arg.wrote_header = FALSE;
+    separate_arg.out_file = fl;
+    cmdstr = "UID FETCH %u (BODY.PEEK[HEADER] BODY.PEEK[TEXT])";
+  } else {
+    handle->body_cb  = write_nstring;
+    handle->body_arg = fl;
+    cmdstr = "UID FETCH %u RFC822";
+  }
+
+  snprintf(cmd, sizeof(cmd), cmdstr, uid);
   rc = imap_cmd_exec(handle, cmd);
+  if(peek) {
+    g_free(separate_arg.body); /* This should never be needed. */
+  }
+
   handle->body_cb  = cb;
   handle->body_arg = arg;
   HANDLE_UNLOCK(handle);
@@ -1100,20 +1225,18 @@
 };
 
 static void
-imap_binary_handler(unsigned seqno, const char *buf,
-                    size_t buflen, void* arg)
+imap_binary_handler(unsigned seqno, ImapFetchBodyType body_type,
+		    const char *buf, size_t buflen, void* arg)
 {
   struct ImapBinaryData *ibd = (struct ImapBinaryData*)arg;
   if(ibd->first_run) {
-    static const char binary_header[] =
-      "Content-Transfer-Encoding: binary\r\n\r\n";
     char *content_type = imap_body_get_content_type(ibd->body);
-    char *str = g_strdup_printf("Content-Type: %s\r\n", content_type);
+    char *str = g_strdup_printf("Content-Type: %s\r\n"
+				"Content-Transfer-Encoding: binary\r\n\r\n",
+				content_type);
     g_free(content_type);
     ibd->body_cb(seqno, str, strlen(str), ibd->body_arg);
     g_free(str);
-    ibd->body_cb(seqno, binary_header, sizeof(binary_header)-1,
-                 ibd->body_arg);
     ibd->first_run = FALSE;
   }
   ibd->body_cb(seqno, buf, buflen, ibd->body_arg);
@@ -1127,10 +1250,11 @@
                             ImapFetchBodyCb body_cb, void *arg)
 {
   char cmd[160];
-  ImapFetchBodyCb fcb = handle->body_cb;
+  ImapFetchBodyInternalCb fcb = handle->body_cb;
   void          *farg = handle->body_arg;
   ImapResponse rc;
   const gchar *peek_string = peek_only ? ".PEEK" : "";
+  struct PassHeaderTextOrdered pass_ordered_data;
 
   IMAP_REQUIRED_STATE1(handle, IMHS_SELECTED, IMR_BAD);
 
@@ -1154,8 +1278,13 @@
       return rc;
     }
   }
-  handle->body_cb  = body_cb;
-  handle->body_arg = arg;
+
+  handle->body_cb  = pass_header_text_ordered;
+  handle->body_arg = &pass_ordered_data;
+  pass_ordered_data.cb = body_cb;
+  pass_ordered_data.arg = arg;
+  pass_ordered_data.body = NULL;
+  pass_ordered_data.wrote_header = FALSE;
   /* Pure IMAP without extensions */
   if(options == IMFB_NONE)
     snprintf(cmd, sizeof(cmd), "FETCH %u BODY%s[%s]",
@@ -1180,6 +1309,7 @@
              seqno, peek_string, prefix, peek_string, section);
   }
   rc = imap_cmd_exec(handle, cmd);
+  g_free(pass_ordered_data.body);
   handle->body_cb  = fcb;
   handle->body_arg = farg;
   return rc;
@@ -1765,7 +1895,8 @@
 }
 
 static void
-msgid_cb(unsigned seqno, const char *buf, size_t buflen, void* arg)
+msgid_cb(unsigned seqno, ImapFetchBodyType body_type,
+	 const char *buf, size_t buflen, void* arg)
 {
   GPtrArray *arr = (GPtrArray*)arg;
   g_return_if_fail(seqno>=1 && seqno<=arr->len);
@@ -1786,7 +1917,7 @@
 {
   gchar *seq, *cmd;
   ImapResponse rc = IMR_OK;
-  ImapFetchBodyCb cb;
+  ImapFetchBodyInternalCb cb;
   void *arg;
 
   IMAP_REQUIRED_STATE1(h, IMHS_SELECTED, IMR_BAD);