From: Kevin McCarthy Date: Sun, 21 May 2017 01:52:16 +0000 (-0700) Subject: Start fixing imap_read_headers() to account for MSN gaps. (see #3942) X-Git-Tag: neomutt-20170707^2^2~42 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=adb35c545384a3aef4d4efbe82ac771b950be6d6;p=neomutt Start fixing imap_read_headers() to account for MSN gaps. (see #3942) Change the parameters to pass MSN instead of index, to make a bit simpler. Change header cache evaluation to look at the largest MSN retrieved instead of ctx->msgcount for the next fetch start point. This still depends on the assumption that MSNs are retrieved in ascending order, which needs to be fixed. Simplify the header cache inner loop termination and memory cleanup logic. Fix a memory leak if a hole in the header cache occured. Fix the header retrieval logic to take into account MSN gaps in the results. Loop only as long as we get IMAP_CMD_CONTINUE instead of over a fixed count. Simplify the inner loop termination and memory cleanup logic too. Simplify the "new mail while fetching" logic by creating a third outer loop to handle re-fetches. Fix msg_fetch_header() to return -2 if msg_parse_fetch() encounters a corrupt FETCH response. Previously it would pass on the rc of msg_parse_fetch(), meaning it would return -1 even though the response was corrupt. --- diff --git a/imap/command.c b/imap/command.c index 71f4fd801..7230da3d9 100644 --- a/imap/command.c +++ b/imap/command.c @@ -290,7 +290,11 @@ void imap_cmd_finish (IMAP_DATA* idata) /* check_status: curs_main uses imap_check_mailbox to detect * whether the index needs updating */ idata->check_status = IMAP_NEWMAIL_PENDING; - imap_read_headers (idata, idata->ctx->msgcount, count-1); + /* TODO: + * The second parameter is wrong. We need to start from + * idata->max_msn + 1. + */ + imap_read_headers (idata, idata->ctx->msgcount+1, count); } else if (idata->reopen & IMAP_EXPUNGE_PENDING) { diff --git a/imap/imap.c b/imap/imap.c index 300010304..2e27e1c54 100644 --- a/imap/imap.c +++ b/imap/imap.c @@ -762,7 +762,7 @@ static int imap_open_mailbox (CONTEXT* ctx) ctx->v2r = safe_calloc (count, sizeof (int)); ctx->msgcount = 0; - if (count && (imap_read_headers (idata, 0, count-1) < 0)) + if (count && (imap_read_headers (idata, 1, count) < 0)) { mutt_error _("Error opening mailbox"); mutt_sleep (1); diff --git a/imap/imap_private.h b/imap/imap_private.h index 4baa26603..67f4dd7ef 100644 --- a/imap/imap_private.h +++ b/imap/imap_private.h @@ -264,7 +264,7 @@ int imap_cmd_idle (IMAP_DATA* idata); /* message.c */ void imap_add_keywords (char* s, HEADER* keywords, LIST* mailbox_flags, size_t slen); void imap_free_header_data (IMAP_HEADER_DATA** data); -int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend); +int imap_read_headers (IMAP_DATA* idata, int msn_begin, int msn_end); char* imap_set_flags (IMAP_DATA* idata, HEADER* h, char* s); int imap_cache_del (IMAP_DATA* idata, HEADER* h); int imap_cache_clean (IMAP_DATA* idata); diff --git a/imap/message.c b/imap/message.c index 563db680d..36758cb94 100644 --- a/imap/message.c +++ b/imap/message.c @@ -71,22 +71,21 @@ static void imap_update_context (IMAP_DATA *idata, int oldmsgcount) /* imap_read_headers: * Changed to read many headers instead of just one. It will return the - * msgno of the last message read. It will return a value other than - * msgend if mail comes in while downloading headers (in theory). + * msn of the last message read. It will return a value other than + * msn_end if mail comes in while downloading headers (in theory). */ -int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) +int imap_read_headers (IMAP_DATA* idata, int msn_begin, int msn_end) { CONTEXT* ctx; char *hdrreq = NULL; FILE *fp; char tempfile[_POSIX_PATH_MAX]; - /* TODO: idx should start at ctx->msgcount */ - int msgno, idx = msgbegin - 1; + int msgno, idx; IMAP_HEADER h; IMAP_STATUS* status; int rc, mfhrc, oldmsgcount; - int fetchlast = 0; - int maxuid = 0; + int fetch_msn_end = 0; + unsigned int maxuid = 0; static const char * const want_headers = "DATE FROM SUBJECT TO CC MESSAGE-ID REFERENCES CONTENT-TYPE CONTENT-DESCRIPTION IN-REPLY-TO REPLY-TO LINES LIST-POST X-LABEL"; progress_t progress; int retval = -1; @@ -130,9 +129,10 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) unlink (tempfile); /* make sure context has room to hold the mailbox */ - while ((msgend) >= idata->ctx->hdrmax) - mx_alloc_memory (idata->ctx); + while (msn_end > ctx->hdrmax) + mx_alloc_memory (ctx); + idx = ctx->msgcount; oldmsgcount = ctx->msgcount; idata->reopen &= ~(IMAP_REOPEN_ALLOW|IMAP_NEWMAIL_PENDING); idata->newMailCount = 0; @@ -140,7 +140,7 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) #if USE_HCACHE idata->hcache = imap_hcache_open (idata, NULL); - if (idata->hcache && !msgbegin) + if (idata->hcache && (msn_begin == 1)) { uid_validity = mutt_hcache_fetch_raw (idata->hcache, "/UIDVALIDITY", imap_hcache_keylen); puidnext = mutt_hcache_fetch_raw (idata->hcache, "/UIDNEXT", imap_hcache_keylen); @@ -158,7 +158,7 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) /* L10N: Comparing the cached data with the IMAP server's data */ mutt_progress_init (&progress, _("Evaluating cache..."), - MUTT_PROGRESS_MSG, ReadInc, msgend + 1); + MUTT_PROGRESS_MSG, ReadInc, msn_end); snprintf (buf, sizeof (buf), "UID FETCH 1:%u (UID FLAGS)", uidnext - 1); @@ -166,47 +166,42 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) imap_cmd_start (idata, buf); rc = IMAP_CMD_CONTINUE; - for (msgno = msgbegin; rc == IMAP_CMD_CONTINUE; msgno++) + for (msgno = 1; rc == IMAP_CMD_CONTINUE; msgno++) { - mutt_progress_update (&progress, msgno + 1, -1); + mutt_progress_update (&progress, msgno, -1); memset (&h, 0, sizeof (h)); h.data = safe_calloc (1, sizeof (IMAP_HEADER_DATA)); do { - mfhrc = 0; + mfhrc = -1; rc = imap_cmd_step (idata); if (rc != IMAP_CMD_CONTINUE) - { - imap_free_header_data (&h.data); break; - } /* hole in the header cache */ if (!evalhc) continue; - if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, NULL)) == -1) + if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, NULL)) < 0) continue; - else if (mfhrc < 0) - { - imap_free_header_data (&h.data); - break; - } if (!h.data->uid) { dprint (2, (debugfile, "imap_read_headers: skipping hcache FETCH " "response for unknown message number %d\n", h.data->msn)); - mfhrc = -1; continue; } - idx++; ctx->hdrs[idx] = imap_hcache_get (idata, h.data->uid); if (ctx->hdrs[idx]) { + /* TODO: This assumes the results arrive in ascending MSN order. + * That is not guaranteed, but the code already has that + * assumption. */ + msn_begin = MAX (msn_begin, h.data->msn + 1); + ctx->hdrs[idx]->index = idx; /* messages which have not been expunged are ACTIVE (borrowed from mh * folders) */ @@ -222,151 +217,151 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) ctx->msgcount++; ctx->size += ctx->hdrs[idx]->content->length; + + h.data = NULL; + idx++; } else { /* bad header in the cache, we'll have to refetch. */ dprint (3, (debugfile, "bad cache entry at MSN %d, giving up\n", h.data->msn)); - imap_free_header_data(&h.data); evalhc = 0; - idx--; } } - while (rc != IMAP_CMD_OK && mfhrc == -1); - if (rc == IMAP_CMD_OK) - break; + while (mfhrc == -1); + + imap_free_header_data (&h.data); + if ((mfhrc < -1) || ((rc != IMAP_CMD_CONTINUE) && (rc != IMAP_CMD_OK))) { - imap_free_header_data (&h.data); imap_hcache_close (idata); goto error_out_1; } } - /* could also look for first null header in case hcache is holey */ - msgbegin = ctx->msgcount; } #endif /* USE_HCACHE */ mutt_progress_init (&progress, _("Fetching message headers..."), - MUTT_PROGRESS_MSG, ReadInc, msgend + 1); + MUTT_PROGRESS_MSG, ReadInc, msn_end); - for (msgno = msgbegin; msgno <= msgend ; msgno++) + while (msn_begin <= msn_end && fetch_msn_end < msn_end) { - mutt_progress_update (&progress, msgno + 1, -1); + char *cmd; - /* we may get notification of new mail while fetching headers */ - if (msgno + 1 > fetchlast) - { - char *cmd; + fetch_msn_end = msn_end; + safe_asprintf (&cmd, "FETCH %d:%d (UID FLAGS INTERNALDATE RFC822.SIZE %s)", + msn_begin, fetch_msn_end, hdrreq); + imap_cmd_start (idata, cmd); + FREE (&cmd); - fetchlast = msgend + 1; - safe_asprintf (&cmd, "FETCH %d:%d (UID FLAGS INTERNALDATE RFC822.SIZE %s)", - msgno + 1, fetchlast, hdrreq); - imap_cmd_start (idata, cmd); - FREE (&cmd); - } - - rewind (fp); - memset (&h, 0, sizeof (h)); - h.data = safe_calloc (1, sizeof (IMAP_HEADER_DATA)); - - /* this DO loop does two things: - * 1. handles untagged messages, so we can try again on the same msg - * 2. fetches the tagged response at the end of the last message. - */ - do + rc = IMAP_CMD_CONTINUE; + for (msgno = msn_begin; rc == IMAP_CMD_CONTINUE; msgno++) { - mfhrc = 0; - - rc = imap_cmd_step (idata); - if (rc != IMAP_CMD_CONTINUE) - break; + mutt_progress_update (&progress, msgno, -1); - if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, fp)) == -1) - continue; - else if (mfhrc < 0) - break; + rewind (fp); + memset (&h, 0, sizeof (h)); + h.data = safe_calloc (1, sizeof (IMAP_HEADER_DATA)); - if (!ftello (fp)) + /* this DO loop does two things: + * 1. handles untagged messages, so we can try again on the same msg + * 2. fetches the tagged response at the end of the last message. + */ + do { - dprint (2, (debugfile, "msg_fetch_header: ignoring fetch response with no body\n")); - mfhrc = -1; - msgend--; - continue; - } - - /* make sure we don't get remnants from older larger message headers */ - fputs ("\n\n", fp); + rc = imap_cmd_step (idata); + if (rc != IMAP_CMD_CONTINUE) + break; - idx++; - if (idx > msgend) - { - dprint (1, (debugfile, "imap_read_headers: skipping FETCH response for " - "unknown message number %d\n", h.data->msn)); - mfhrc = -1; - idx--; - continue; - } - /* May receive FLAGS updates in a separate untagged response (#2935) */ - if (idx < ctx->msgcount) - { - dprint (2, (debugfile, "imap_read_headers: message %d is not new\n", - h.data->msn)); - idx--; - continue; - } + if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, fp)) < 0) + continue; - ctx->hdrs[idx] = mutt_new_header (); + if (!ftello (fp)) + { + dprint (2, (debugfile, "msg_fetch_header: ignoring fetch response with no body\n")); + continue; + } - ctx->hdrs[idx]->index = idx; - /* messages which have not been expunged are ACTIVE (borrowed from mh - * folders) */ - ctx->hdrs[idx]->active = 1; - ctx->hdrs[idx]->read = h.data->read; - ctx->hdrs[idx]->old = h.data->old; - ctx->hdrs[idx]->deleted = h.data->deleted; - ctx->hdrs[idx]->flagged = h.data->flagged; - ctx->hdrs[idx]->replied = h.data->replied; - ctx->hdrs[idx]->changed = h.data->changed; - ctx->hdrs[idx]->received = h.received; - ctx->hdrs[idx]->data = (void *) (h.data); + /* make sure we don't get remnants from older larger message headers */ + fputs ("\n\n", fp); - if (maxuid < h.data->uid) - maxuid = h.data->uid; + if (h.data->msn < msn_begin || h.data->msn > fetch_msn_end) + { + dprint (1, (debugfile, "imap_read_headers: skipping FETCH response for " + "unknown message number %d\n", h.data->msn)); + continue; + } - rewind (fp); - /* NOTE: if Date: header is missing, mutt_read_rfc822_header depends - * on h.received being set */ - ctx->hdrs[idx]->env = mutt_read_rfc822_header (fp, ctx->hdrs[idx], - 0, 0); - /* content built as a side-effect of mutt_read_rfc822_header */ - ctx->hdrs[idx]->content->length = h.content_length; - ctx->size += h.content_length; + /* May receive FLAGS updates in a separate untagged response (#2935) */ + /* + * TODO: This check was introduced in e5c2befbf0f5 + * But was broken in 0e4f1782ea2e + * We need to check against idata->msn_index to see if + * we already have a header downloaded, when that is introduced. + if (idx < ctx->msgcount) + { + dprint (2, (debugfile, "imap_read_headers: message %d is not new\n", + h.data->msn)); + continue; + } + */ + + ctx->hdrs[idx] = mutt_new_header (); + + ctx->hdrs[idx]->index = idx; + /* messages which have not been expunged are ACTIVE (borrowed from mh + * folders) */ + ctx->hdrs[idx]->active = 1; + ctx->hdrs[idx]->read = h.data->read; + ctx->hdrs[idx]->old = h.data->old; + ctx->hdrs[idx]->deleted = h.data->deleted; + ctx->hdrs[idx]->flagged = h.data->flagged; + ctx->hdrs[idx]->replied = h.data->replied; + ctx->hdrs[idx]->changed = h.data->changed; + ctx->hdrs[idx]->received = h.received; + ctx->hdrs[idx]->data = (void *) (h.data); + + if (maxuid < h.data->uid) + maxuid = h.data->uid; + + rewind (fp); + /* NOTE: if Date: header is missing, mutt_read_rfc822_header depends + * on h.received being set */ + ctx->hdrs[idx]->env = mutt_read_rfc822_header (fp, ctx->hdrs[idx], + 0, 0); + /* content built as a side-effect of mutt_read_rfc822_header */ + ctx->hdrs[idx]->content->length = h.content_length; + ctx->size += h.content_length; #if USE_HCACHE - imap_hcache_put (idata, ctx->hdrs[idx]); + imap_hcache_put (idata, ctx->hdrs[idx]); #endif /* USE_HCACHE */ - ctx->msgcount++; - } - while ((rc != IMAP_CMD_OK) && ((mfhrc == -1) || - ((msgno + 1) >= fetchlast))); + ctx->msgcount++; + + h.data = NULL; + idx++; + } + while (mfhrc == -1); - if ((mfhrc < -1) || ((rc != IMAP_CMD_CONTINUE) && (rc != IMAP_CMD_OK))) - { imap_free_header_data (&h.data); + + if ((mfhrc < -1) || ((rc != IMAP_CMD_CONTINUE) && (rc != IMAP_CMD_OK))) + { #if USE_HCACHE - imap_hcache_close (idata); + imap_hcache_close (idata); #endif - goto error_out_1; + goto error_out_1; + } } /* in case we get new mail while fetching the headers */ if (idata->reopen & IMAP_NEWMAIL_PENDING) { - msgend = idata->newMailCount - 1; - while ((msgend) >= ctx->hdrmax) - mx_alloc_memory (ctx); + msn_begin = fetch_msn_end + 1; + msn_end = idata->newMailCount; + while (msn_end > ctx->hdrmax) + mx_alloc_memory (ctx); idata->reopen &= ~IMAP_NEWMAIL_PENDING; idata->newMailCount = 0; } @@ -393,6 +388,8 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) if (ctx->msgcount > oldmsgcount) { + /* TODO: it's not clear to me why we are calling mx_alloc_memory + * yet again. */ mx_alloc_memory(ctx); mx_update_context (ctx, ctx->msgcount - oldmsgcount); imap_update_context (idata, oldmsgcount); @@ -400,7 +397,7 @@ int imap_read_headers (IMAP_DATA* idata, int msgbegin, int msgend) idata->reopen |= IMAP_REOPEN_ALLOW; - retval = msgend; + retval = msn_end; error_out_1: safe_fclose (&fp); @@ -1132,6 +1129,7 @@ static int msg_fetch_header (CONTEXT* ctx, IMAP_HEADER* h, char* buf, FILE* fp) IMAP_DATA* idata; long bytes; int rc = -1; /* default now is that string isn't FETCH response*/ + int parse_rc; idata = (IMAP_DATA*) ctx->data; @@ -1154,7 +1152,10 @@ static int msg_fetch_header (CONTEXT* ctx, IMAP_HEADER* h, char* buf, FILE* fp) /* FIXME: current implementation - call msg_parse_fetch - if it returns -2, * read header lines and call it again. Silly. */ - if ((rc = msg_parse_fetch (h, buf)) != -2 || !fp) + parse_rc = msg_parse_fetch (h, buf); + if (!parse_rc) + return 0; + if (parse_rc != -2 || !fp) return rc; if (imap_get_literal_count (buf, &bytes) == 0) @@ -1181,7 +1182,13 @@ static int msg_fetch_header (CONTEXT* ctx, IMAP_HEADER* h, char* buf, FILE* fp) return rc; } -/* msg_parse_fetch: handle headers returned from header fetch */ +/* msg_parse_fetch: handle headers returned from header fetch. + * Returns: + * 0 on success + * -1 if the string is corrupted + * -2 if the fetch contains a body or header lines + * that still need to be parsed. + */ static int msg_parse_fetch (IMAP_HEADER *h, char *s) { char tmp[SHORT_STRING];