From 77acc268123b8233843ca9bc3dcf90669efde08f Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Sun, 18 Dec 2016 20:50:20 +0100 Subject: [PATCH] implement Message-Id based UIDVALIDITY recovery --- NEWS | 2 ++ README | 10 --------- src/driver.c | 1 + src/driver.h | 3 +++ src/drv_imap.c | 55 ++++++++++++++++++++++++++++++++++++++++------ src/drv_maildir.c | 50 +++++++++++++++++++++++++++++++++++++----- src/mbsync.1 | 4 ++-- src/sync.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- 8 files changed, 150 insertions(+), 31 deletions(-) diff --git a/NEWS b/NEWS index 5c74ce1..ebb88a4 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,8 @@ A Maildir sub-folder naming style without extra dots has been added. Support for TLS client certificates was added. +Support for recovering from baseless UID validity changes was added. + [1.2.0] The 'isync' compatibility wrapper is now deprecated. diff --git a/README b/README index ffa6291..abb873d 100644 --- a/README +++ b/README @@ -45,16 +45,6 @@ isync executable still exists; it is a compatibility wrapper around mbsync. Courier 1.4.3 is known to be buggy, version 1.7.3 works fine. - c-client (UW-IMAP, Pine) is mostly fine, but versions less than 2004a.352 - tend to change UIDVALIDITY pretty often when used with unix/mbox mailboxes, - making isync refuse synchronization. - M$ Exchange (up to 2010 at least) occasionally exposes the same problem. - The "cure" is to simply copy the new UIDVALIDITY from the affected - mailbox to mbsync's state file. This is a Bad Hack (TM), but it works - - use at your own risk (if the UIDVALIDITY change was genuine, this will - delete all messages in the affected mailbox - not that this ever - happened to me). - M$ Exchange (2013 at least) needs DisableExtension MOVE to be compatible with the Trash functionality. diff --git a/src/driver.c b/src/driver.c index 86c172b..e522d14 100644 --- a/src/driver.c +++ b/src/driver.c @@ -34,6 +34,7 @@ free_generic_messages( message_t *msgs ) for (; msgs; msgs = tmsg) { tmsg = msgs->next; + free( msgs->msgid ); free( msgs ); } } diff --git a/src/driver.h b/src/driver.h index d9e5f4c..a97c231 100644 --- a/src/driver.h +++ b/src/driver.h @@ -63,6 +63,7 @@ typedef struct store_conf { typedef struct message { struct message *next; struct sync_rec *srec; + char *msgid; /* owned */ /* string_list_t *keywords; */ size_t size; /* zero implies "not fetched" */ int uid; @@ -80,6 +81,7 @@ typedef struct message { #define OPEN_SETFLAGS (1<<6) #define OPEN_APPEND (1<<7) #define OPEN_FIND (1<<8) +#define OPEN_OLD_IDS (1<<9) typedef struct store { struct store *next; @@ -208,6 +210,7 @@ struct driver { * and those named in the excs array (smaller than minuid). * The driver takes ownership of the excs array. * Messages starting with newuid need to have the TUID populated when OPEN_FIND is set. + * Messages up to seenuid need to have the Message-Id populated when OPEN_OLD_IDS is set. * Messages up to seenuid need to have the size populated when OPEN_OLD_SIZE is set; * likewise messages above seenuid when OPEN_NEW_SIZE is set. */ void (*load_box)( store_t *ctx, int minuid, int maxuid, int newuid, int seenuid, int_array_t excs, diff --git a/src/drv_imap.c b/src/drv_imap.c index 88ff50e..e24c7d8 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -905,7 +905,7 @@ static int parse_fetch_rsp( imap_store_t *ctx, list_t *list, char *s ATTR_UNUSED ) { list_t *tmp, *flags; - char *body = 0, *tuid = 0; + char *body = 0, *tuid = 0, *msgid = 0; imap_message_t *cur; msg_data_t *msgdata; struct imap_cmd *cmdp; @@ -983,8 +983,42 @@ parse_fetch_rsp( imap_store_t *ctx, list_t *list, char *s ATTR_UNUSED ) tmp = tmp->next; if (!is_atom( tmp )) goto bfail; - if (starts_with_upper( tmp->val, tmp->len, "X-TUID: ", 8 )) - tuid = tmp->val + 8; + int off, in_msgid = 0; + for (char *val = tmp->val, *end; (end = strchr( val, '\n' )); val = end + 1) { + int len = (int)(end - val); + if (len && end[-1] == '\r') + len--; + if (!len) + break; + if (starts_with_upper( val, len, "X-TUID: ", 8 )) { + if (len < 8 + TUIDL) { + error( "IMAP error: malformed X-TUID header (UID %d)\n", uid ); + continue; + } + tuid = val + 8; + in_msgid = 0; + continue; + } + if (starts_with_upper( val, len, "MESSAGE-ID:", 11 )) { + off = 11; + } else if (in_msgid) { + if (!isspace( val[0] )) { + in_msgid = 0; + continue; + } + off = 1; + } else { + continue; + } + while (off < len && isspace( val[off] )) + off++; + if (off == len) { + in_msgid = 1; + continue; + } + msgid = nfstrndup( val + off, len - off ); + in_msgid = 0; + } } else { bfail: error( "IMAP error: unable to parse BODY[HEADER.FIELDS ...]\n" ); @@ -1018,6 +1052,7 @@ parse_fetch_rsp( imap_store_t *ctx, list_t *list, char *s ATTR_UNUSED ) cur->gen.status = status; cur->gen.size = size; cur->gen.srec = 0; + cur->gen.msgid = msgid; if (tuid) strncpy( cur->gen.tuid, tuid, TUIDL ); else @@ -2322,7 +2357,7 @@ imap_prepare_load_box( store_t *gctx, int opts ) gctx->opts = opts; } -enum { WantSize = 1, WantTuids = 2 }; +enum { WantSize = 1, WantTuids = 2, WantMsgids = 4 }; typedef struct imap_range { int first, last, flags; } imap_range_t; @@ -2375,7 +2410,7 @@ imap_load_box( store_t *gctx, int minuid, int maxuid, int newuid, int seenuid, i if (i != j) bl += sprintf( buf + bl, ":%d", excs.data[i] ); } - imap_submit_load( ctx, buf, 0, sts ); + imap_submit_load( ctx, buf, shifted_bit( ctx->gen.opts, OPEN_OLD_IDS, WantMsgids ), sts ); } if (maxuid == INT_MAX) maxuid = ctx->gen.uidnext ? ctx->gen.uidnext - 1 : 0x7fffffff; @@ -2390,6 +2425,8 @@ imap_load_box( store_t *gctx, int minuid, int maxuid, int newuid, int seenuid, i shifted_bit( ctx->gen.opts, OPEN_NEW_SIZE, WantSize), seenuid ); if (ctx->gen.opts & OPEN_FIND) imap_set_range( ranges, &nranges, 0, WantTuids, newuid - 1 ); + if (ctx->gen.opts & OPEN_OLD_IDS) + imap_set_range( ranges, &nranges, WantMsgids, 0, seenuid ); for (int r = 0; r < nranges; r++) { sprintf( buf, "%d:%d", ranges[r].first, ranges[r].last ); imap_submit_load( ctx, buf, ranges[r].flags, sts ); @@ -2404,10 +2441,14 @@ static void imap_submit_load( imap_store_t *ctx, const char *buf, int flags, struct imap_cmd_refcounted_state *sts ) { imap_exec( ctx, imap_refcounted_new_cmd( sts ), imap_refcounted_done_box, - "UID FETCH %s (UID%s%s%s)", buf, + "UID FETCH %s (UID%s%s%s%s%s%s%s)", buf, (ctx->gen.opts & OPEN_FLAGS) ? " FLAGS" : "", (flags & WantSize) ? " RFC822.SIZE" : "", - (flags & WantTuids) ? " BODY.PEEK[HEADER.FIELDS (X-TUID)]" : ""); + (flags & (WantTuids | WantMsgids)) ? " BODY.PEEK[HEADER.FIELDS (" : "", + (flags & WantTuids) ? "X-TUID" : "", + !(~flags & (WantTuids | WantMsgids)) ? " " : "", + (flags & WantMsgids) ? "MESSAGE-ID" : "", + (flags & (WantTuids | WantMsgids)) ? ")]" : ""); } /******************* imap_fetch_msg *******************/ diff --git a/src/drv_maildir.c b/src/drv_maildir.c index 64ed069..d174bdd 100644 --- a/src/drv_maildir.c +++ b/src/drv_maildir.c @@ -442,6 +442,7 @@ static const char *subdirs[] = { "cur", "new", "tmp" }; typedef struct { char *base; + char *msgid; int size; uint uid:31, recent:1; char tuid[TUIDL]; @@ -926,6 +927,7 @@ maildir_scan( maildir_store_t *ctx, msg_t_array_alloc_t *msglist ) continue; entry = msg_t_array_append( msglist ); entry->base = nfstrdup( e->d_name ); + entry->msgid = 0; entry->uid = uid; entry->recent = i; entry->size = 0; @@ -1050,7 +1052,8 @@ maildir_scan( maildir_store_t *ctx, msg_t_array_alloc_t *msglist ) } int want_size = (uid > ctx->seenuid) ? (ctx->gen.opts & OPEN_NEW_SIZE) : (ctx->gen.opts & OPEN_OLD_SIZE); int want_tuid = ((ctx->gen.opts & OPEN_FIND) && uid >= ctx->newuid); - if (!want_size && !want_tuid) + int want_msgid = ((ctx->gen.opts & OPEN_OLD_IDS) && uid <= ctx->seenuid); + if (!want_size && !want_tuid && !want_msgid) continue; if (!fnl) nfsnprintf( buf + bl, sizeof(buf) - bl, "%s/%s", subdirs[entry->recent], entry->base ); @@ -1064,7 +1067,7 @@ maildir_scan( maildir_store_t *ctx, msg_t_array_alloc_t *msglist ) } entry->size = st.st_size; } - if (want_tuid) { + if (want_tuid || want_msgid) { if (!(f = fopen( buf, "r" ))) { if (errno != ENOENT) { sys_error( "Maildir error: cannot open %s", buf ); @@ -1072,13 +1075,45 @@ maildir_scan( maildir_store_t *ctx, msg_t_array_alloc_t *msglist ) } goto retry; } - while (fgets( nbuf, sizeof(nbuf), f )) { - if (!nbuf[0] || nbuf[0] == '\n') + int off, in_msgid = 0; + while ((want_tuid || want_msgid) && fgets( nbuf, sizeof(nbuf), f )) { + int bufl = strlen( nbuf ); + if (bufl && nbuf[bufl - 1] == '\n') + --bufl; + if (bufl && nbuf[bufl - 1] == '\r') + --bufl; + if (!bufl) break; - if (starts_with( nbuf, -1, "X-TUID: ", 8 ) && nbuf[8 + TUIDL] == '\n') { + if (want_tuid && starts_with( nbuf, bufl, "X-TUID: ", 8 )) { + if (bufl < 8 + TUIDL) { + error( "Maildir error: malformed X-TUID header (UID %d)\n", uid ); + continue; + } memcpy( entry->tuid, nbuf + 8, TUIDL ); - break; + want_tuid = 0; + in_msgid = 0; + continue; } + if (want_msgid && starts_with_upper( nbuf, bufl, "MESSAGE-ID:", 11 )) { + off = 11; + } else if (in_msgid) { + if (!isspace( nbuf[0] )) { + in_msgid = 0; + continue; + } + off = 1; + } else { + continue; + } + while (off < bufl && isspace( nbuf[off] )) + off++; + if (off == bufl) { + in_msgid = 1; + continue; + } + entry->msgid = nfstrndup( nbuf + off, bufl - off ); + want_msgid = 0; + in_msgid = 0; } fclose( f ); } @@ -1093,6 +1128,8 @@ maildir_init_msg( maildir_store_t *ctx, maildir_message_t *msg, msg_t *entry ) { msg->base = entry->base; entry->base = 0; /* prevent deletion */ + msg->gen.msgid = entry->msgid; + entry->msgid = 0; /* prevent deletion */ msg->gen.size = entry->size; msg->gen.srec = 0; strncpy( msg->gen.tuid, entry->tuid, TUIDL ); @@ -1350,6 +1387,7 @@ maildir_rescan( maildir_store_t *ctx ) debug( "updating message %d\n", msg->gen.uid ); msg->gen.status &= ~(M_FLAGS|M_RECENT); free( msg->base ); + free( msg->gen.msgid ); maildir_init_msg( ctx, msg, msglist.array.data + i ); i++, msgapp = &msg->gen.next; } diff --git a/src/mbsync.1 b/src/mbsync.1 index 00a8048..627181e 100644 --- a/src/mbsync.1 +++ b/src/mbsync.1 @@ -36,8 +36,8 @@ the operation set can be selected in a fine-grained manner. .br Synchronization is based on unique message identifiers (UIDs), so no identification conflicts can occur (unlike with some other mail synchronizers). -OTOH, \fBmbsync\fR is susceptible to UID validity changes (that \fIshould\fR -never happen, but see "Compatibility" in the README). +OTOH, \fBmbsync\fR is susceptible to UID validity changes (but will recover +just fine if the change is unfounded). Synchronization state is kept in one local text file per mailbox pair; these files are protected against concurrent \fBmbsync\fR processes. Mailboxes can be safely modified while \fBmbsync\fR operates diff --git a/src/sync.c b/src/sync.c index 2a61abd..eb48dc5 100644 --- a/src/sync.c +++ b/src/sync.c @@ -1165,12 +1165,13 @@ box_opened2( sync_vars_t *svars, int t ) fails = 0; for (t = 0; t < 2; t++) - if (svars->uidval[t] >= 0 && svars->uidval[t] != ctx[t]->uidvalidity) { - error( "Error: UIDVALIDITY of %s changed (got %d, expected %d)\n", - str_ms[t], ctx[t]->uidvalidity, svars->uidval[t] ); + if (svars->uidval[t] >= 0 && svars->uidval[t] != ctx[t]->uidvalidity) fails++; - } - if (fails) { + if (fails == 2) { + error( "Error: channel %s: UIDVALIDITY of both master and slave changed\n" + "(master got %d, expected %d; slave got %d, expected %d).\n", + svars->chan->name, + ctx[M]->uidvalidity, svars->uidval[M], ctx[S]->uidvalidity, svars->uidval[S] ); bail: svars->ret = SYNC_FAIL; sync_bail( svars ); @@ -1193,6 +1194,8 @@ box_opened2( sync_vars_t *svars, int t ) Fprintf( svars->jfp, JOURNAL_VERSION "\n" ); opts[M] = opts[S] = 0; + if (fails) + opts[M] = opts[S] = OPEN_OLD|OPEN_OLD_IDS; for (t = 0; t < 2; t++) { if (chan->ops[t] & (OP_DELETE|OP_FLAGS)) { opts[t] |= OPEN_SETFLAGS; @@ -1323,7 +1326,7 @@ load_box( sync_vars_t *svars, int t, int minwuid, int_array_t mexcs ) if (minwuid > svars->maxuid[t] + 1) minwuid = svars->maxuid[t] + 1; maxwuid = INT_MAX; - if (svars->ctx[t]->opts & OPEN_OLD_SIZE) + if (svars->ctx[t]->opts & (OPEN_OLD_IDS|OPEN_OLD_SIZE)) seenuid = get_seenuid( svars, t ); else seenuid = 0; @@ -1429,6 +1432,47 @@ box_loaded( int sts, void *aux ) if (!(svars->state[1-t] & ST_LOADED)) return; + for (t = 0; t < 2; t++) { + if (svars->uidval[t] >= 0 && svars->uidval[t] != svars->ctx[t]->uidvalidity) { + unsigned need = 0, got = 0; + debug( "trying to re-approve uid validity of %s\n", str_ms[t] ); + for (srec = svars->srecs; srec; srec = srec->next) { + if (srec->status & S_DEAD) + continue; + if (!srec->msg[t]) + continue; // Message disappeared. + need++; // Present paired messages require re-validation. + if (!srec->msg[t]->msgid) + continue; // Messages without ID are useless for re-validation. + if (!srec->msg[1-t]) + continue; // Partner disappeared. + if (!srec->msg[1-t]->msgid || strcmp( srec->msg[M]->msgid, srec->msg[S]->msgid )) { + error( "Error: channel %s, %s %s: UIDVALIDITY genuinely changed (at UID %d).\n", + svars->chan->name, str_ms[t], svars->orig_name[t], srec->uid[t] ); + uvchg: + svars->ret |= SYNC_FAIL; + cancel_sync( svars ); + return; + } + got++; + } + if (got < 20 && got * 5 < need * 4) { + // Too few confirmed messages. This is very likely in the drafts folder. + // A proper fallback would be fetching more headers (which potentially need + // normalization) or the message body (which should be truncated for sanity) + // and comparing. + error( "Error: channel %s, %s %s: Unable to recover from UIDVALIDITY change\n" + "(got %d, expected %d).\n", + svars->chan->name, str_ms[t], svars->orig_name[t], + svars->ctx[t]->uidvalidity, svars->uidval[t] ); + goto uvchg; + } + notice( "Notice: channel %s, %s %s: Recovered from change of UIDVALIDITY.\n", + svars->chan->name, str_ms[t], svars->orig_name[t] ); + svars->uidval[t] = -1; + } + } + if (svars->uidval[M] < 0 || svars->uidval[S] < 0) { svars->uidval[M] = svars->ctx[M]->uidvalidity; svars->uidval[S] = svars->ctx[S]->uidvalidity;