[Akonadi] [Bug 319450] New: Mailbox synchronization can miss important mailbox state updates -- broken EXISTS and UIDNEXT heuristic

[Jan Kundrát]

https://bugs.kde.org/show_bug.cgi?id=319450

            Bug ID: 319450
           Summary: Mailbox synchronization can miss important mailbox
                    state updates -- broken EXISTS and UIDNEXT heuristic
    Classification: Unclassified
           Product: Akonadi
           Version: GIT (master)
          Platform: Other
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: NOR
         Component: IMAP resource
          Assignee: ervin at kde.org
          Reporter: jkt at flaska.net
                CC: kdepim-bugs at kde.org, vkrause at kde.org

This is how the current kdepim-runtime/resources/imap/retrieveitemstask.cpp
looks like:


  } else if ( messageCount > realMessageCount && messageCount > 0 ) {
    // The amount on the server is bigger than that we have in the cache
    // that probably means that there is new mail. Fetch missing.

    kDebug( 5327 ) << "Fetch missing: " << messageCount << " But: " <<
realMessageCount;

    KIMAP::FetchJob *fetch = new KIMAP::FetchJob( m_session );
    fetch->setSequenceSet( KIMAP::ImapSet( realMessageCount+1, messageCount )
);


If I understand this correctly and it really sends ``FETCH oldExists+1:*`` over
the wire, then it's broken -- it will fail to notice any deletions among the
old messages, for example.

Goging further, the next branch apparently assumes that UIDs are assigned on a
consecutive manner, i.e. after 123, there will always be 124. This is wrong;
the UIDNEXT as provided by the IMAP server is a lower bound on what the next
visible UID might be. It is perfectly safe to have messages with assigned UIDs
10, 15, 21 etc from the very beginning (not to mention a possible removal on
the server side from a concurrent client while Akonadi is offline).

  } else if ( messageCount == realMessageCount && oldNextUid != nextUid
           && oldNextUid != 0 && !firstTime && messageCount > 0 ) {
    // amount is right but uidnext is different.... something happened
    // behind our back...
    kDebug( 5327 ) << "UIDNEXT check failed, refetching mailbox";

    qint64 startIndex = 1;
    // one scenario we can recover from is that an equal amount of mails has
been deleted and added while we were not looking
    // the amount has to be less or equal to (nextUid - oldNextUid) due to
strictly ascending UIDs
    // so, we just have to reload the last (nextUid - oldNextUid) mails if the
uidnext values seem sane
    if ( oldNextUid < nextUid && oldNextUid != 0 && !firstTime )
      startIndex = qMax( 1ll, messageCount - ( nextUid - oldNextUid ) );


This is wrong -- if the UIDNEXT was 10 previosuly and is now 11, the code will
do a FETCH 8:*. However, there is no guaranteee whatsoever that the removal was
not for, let's say, message with seq. number 1.

This is based just on reading the synchronization code, I have not performed
any real tests, but it looks like testretrieveitemstask.cpp does not have a
test coverage for this situation.

There's quite a lot of testcases for this in Trojita; please feel free to get
in touch if you cannot find them.

Reproducible: Always

-- 
You are receiving this mail because:
You are on the CC list for the bug.