Cyrus 2.3.16 \Seen flag problem

[Julien Coloos]

Hi,

We encountered a bug in cyrus 2.3.16: mails freshly delivered by LMTP
were already flagged \Seen while not having been read by anyone.
After some investigations, we tracked it down to the code in charge of
updating the seen db: function index_checkseen in imap/index.c file.
That complicated code seems quite prone to issues considering that 5
sets of data are handled:
 - the 'old' seen db, listing UIDs with its initial \Seen state
 - an array in which \Seen state after actions in the current session are stored
 - the 'new' seen db, loaded right before updating its content (to
merge any concurrent sessions changes)
 - the index file data held in static variables of the index.c code section
   -> lets call it index1
 - the mailbox structure, with a possibly newer version of the index file data
   -> lets call it index2

The steps to trigger the issue:
1. The INBOX is opened
2. Mails not already flagged \Seen are read (and so the \Seen flag is
set); alternatively mails are not read but the \Seen flag is set on
them
3. One or more mails are delivered to the mailbox through LMTP
4. The INBOX is closed (with implicit expunge)
Note: using the 'UID' version of the commands (FETCH/STORE) lowers the
probability of encountering the issue because index1 is refreshed for
those commands.

Why those steps trigger the issue ?
Upon closing the INBOX folder, an implicit expunge is performed; a
side effect is that index2 is refreshed. With the other conditions
met:
 - all known mails (not taking into account the ones delivered by
LMTP, which are not visible in index1) now have the \Seen flag
 - the 'old' and 'new' seen db do not differ
the current code updates the seen db believing that UIDs 1 up to
mailbox->last_uid have the \Seen flag. The problem is that mailbox was
refeshed and so all mails delivered by LMTP are now flagged \Seen.

Following the source code with those conditions we would get the
following values:
 - newseen = 0
 - newnext = mailbox->last_uid + 1
 - neweof = 1
 - start = 1
 - inrange = 1
 - usecomma = 0
 - uid = mailbox->last_uid

So we would end in the following part of the code:

    if (inrange) {
	/* Last message read. */
...
	if (!start && uid > 1) start = 1;
	if (usecomma++) *save++ = ',';
	if (start && start != uid) {
	    sprintf(save, "%u:", start);
	    save += strlen(save);
	}
	sprintf(save, "%u", uid);
	save += strlen(save);
...

which does output the range [1,mailbox->last_uid] in the seen db.


It may be possible that some other set of conditions would trigger a
similar issue.
We tried to think of a clean way to solve it, but provided the
complexity of that code we gave up (mainly fearing nasty side effetcs
and introducing more bugs).
Maybe that's the reason it was completely rewritten in cyrus 2.4.x
(which does not have that bug) ? ;)
Unfortunately, moving to 2.4.x version is not yet an option for us :(

So ... is there some cyrus guru expert out there with enough knowledge
of that part of the code to fix it in the 2.3.x version ?


Regards