[PATCH] Fix message corruption with concurrent flag changes

[X Ryl]

Applied in my next branch, if no one objects, I'll apply to the official
repo next branch tomorrow.

On Mon, Feb 4, 2013 at 5:52 AM, Austin Clements <amdragon at mit.edu> wrote:

> Ping.
>
> I've been running with this patch for some time now and haven't received
> any more corrupted email deliveries (though my logs show many instances
> where I would have received a corrupted email with the unpatched code).
>
> On Sat, 29 Dec 2012, Austin Clements <amdragon at MIT.EDU> wrote:
> > IMAPFolder.getmessage incorrectly assumes that the 'data' returned by
> > imapobj.uid('fetch', ...) will always be of the form
> >
> >   [(fetch-info, message-body)]
> >
> > However, this call returns a list of *all* pending untagged FETCH
> > responses, not just the requested FETCH.  If any message flags were
> > changed in the selected IMAP folder since the last command (by another
> > client or another thread in OfflineIMAP itself), the IMAP server will
> > issue unsolicited FETCH responses indicating these flag changes
> > (RFC3501, section 7).  When this happens, 'data' will look like, for
> > example
> >
> >   ['1231 (FLAGS (\\Seen) UID 5300)',
> >    '1238 (FLAGS (\\Seen) UID 5318)',
> >    ('1242 (UID 5325 BODY[] {7976}', message-body)]
> >
> > Unfortunately, getmessage retrieves the message body as data[0][1],
> > which in this example is just the string "2".  This is what ultimately
> > gets stored in the maildir file, resulting in a message consisting
> > exclusively of a single ASCII digit.
> >
> > Ideally, either imaplib2 or getmessage would parse the fetch responses
> > to find the response for the requested UID.  However, since IMAP only
> > specifies unilateral FETCH responses for flag changes, the body fetch
> > will be the only tuple in the response; hence this patch simply finds
> > the element of 'data' that is a tuple (aborting if there is more than
> > one tuple) and uses that.
> >
> > Multi-threaded OfflineIMAP with IDLE or holdconnectionopen is
> > particularly susceptible to this problem because flag changes synced
> > back to the IMAP server on one thread will appear as unsolicited FETCH
> > responses on another thread if it happens to have the same folder
> > selected.  This can also happen without IDLE or holdconnectionopen or
> > even in single-threaded OfflineIMAP with concurrent access from other
> > IMAP clients (webmail clients, etc.), though the window for the bug is
> > much smaller.
> >
> > Signed-off-by: Austin Clements <amdragon at mit.edu>
> > ---
> >
> > I posted a preliminary version of this patch on December 7th.  I
> > tweaked it slightly in the following days and have been running this
> > continuously since then without any problems.
> >
> > Since the context of this patch has changed dramatically between maint
> > and master, this is based on master.  maint also has this bug,
> > however, and if people want I can write a separate backported patch.
> >
> >  offlineimap/folder/IMAP.py |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py
> > index 75520c2..6b0ed04 100644
> > --- a/offlineimap/folder/IMAP.py
> > +++ b/offlineimap/folder/IMAP.py
> > @@ -226,7 +226,15 @@ class IMAPFolder(BaseFolder):
> >                      fails_left -= 1
> >                      if not fails_left:
> >                          raise e
> > -            if data == [None] or res_type != 'OK':
> > +            # Remove unsolicited FETCH responses caused by flag
> > +            # changes from concurrent connections.  These appear as
> > +            # strings in 'data', while the BODY response appears as a
> > +            # tuple.  The closing ')' on the BODY response also
> > +            # appears as a stray string, which this also removes.
> > +            # This should leave exactly one response in 'data'.
> > +            if res_type == 'OK':
> > +                data = [res for res in data if not isinstance(res, str)]
> > +            if data == [None] or res_type != 'OK' or len(data) != 1:
> >                  #IMAP server says bad request or UID does not exist
> >                  severity = OfflineImapError.ERROR.MESSAGE
> >                  reason = "IMAP server '%s' failed to fetch message UID
> '%d'."\
> > --
> > 1.7.10.4
>
> _______________________________________________
> OfflineIMAP-project mailing list
> OfflineIMAP-project at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/offlineimap-project
>
> OfflineIMAP homepage: http://software.complete.org/offlineimap
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20130204/c2623ce6/attachment-0003.html>