[PATCH] Fix message corruption with concurrent flag changes

[Austin Clements]

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