Re: [Imap-protocol] Re: UW-IMAPD and its interpretation of ESEARCH

On Sat, 8 Jun 2013, Jan Kundrát wrote:

> On Saturday, 8 June 2013 08:10:48 CEST, Jonathan Abbey wrote:
>>   http://github.com/jonabey/panda-imap
>
> Assuming your github username is with double Bs, the offending code is likely 
> found at 
> https://github.com/jonabbey/panda-imap/blob/master/src/imapd/imapd.c#L939 . 
> I'm not sure whether I'm interpreting it correctly, it's a foreign code with 
> a very different style than I'm used to.

True, Mark had a coding style that is recognizable by anybody who learned
programming 35 (or more ;) years ago. (when systems had 64KB of main memory
and 5MB of disk space, programmers needed an economy of style that bordered on
terse).

> That code appears to be broken on a fourth read or so :) -- it iterates over 
> all messages and if a particular sequence of messages matches, it will output 
> the result as min:max. That is, however, the exact problem here -- the code 
> does not check whether the range of UIDs assigned to these messages has any 
> holes within. In other words, the code produces correct results for SEARCH 
> RETURN ..., but fails for UID SEARCH RETURN ... because the UIDs, unlike 
> sequence numbers, are not guaranteed to be consecutive.

Mark's long standing contention was that the ESEARCH rfc allowed his
interpretation, so his code wasn't broken, just different.
As he's no longer around to defend his position I'm not going to comment
on it. Anybody who's interested can read the ietf.org mail archives.

> This is consistent with the output of `git blame` which suggests that the 
> code in question comes from commit 199adc96 which is marked as 
> "imap-2006i.tar.Z" in your repository. The capability advertised by the 
> user's server says 2007e.404, and that version is still affected.
>
> You can easily check this yourself. Create new mailbox, append three 
> messages, remove the middle one and send `UID SEARCH RETURN (ALL) ALL`. If 
> you get back something like "1,3", the code is OK, if you get "1:3", it's 
> wrong.
>
> I do not dare writing a patch for that code -- the assumption that one could 
> get away with acting as if the client knew which UIDs exist and which are 
> gone is a big one. The loop at line 952 looks like the obvious problem (it 
> shall check for a consecutive range of UIDs, but only if the command is the 
> UID variant of ESEARCH...), but more changes might be needed. Well, I've had 
> enough C for now :).

As somebody who has over 15 years experience hacking Mark's imap code, I feel
comfortable creating a patch for that section to bring its behavior inline
with what the rest of the world thinks it should do. See attached patch file.
It should apply to Panda-imap & UW-IMAPD 2007e.

Jon,
Would you please consider adding this patch to your project.

> With kind regards,
> Jan
>
> [1] http://tools.ietf.org/html/rfc3676

-- 
Dave Funk                                  University of Iowa
<dbfunk (at) engineering.uiowa.edu>        College of Engineering
319/335-5751   FAX: 319/384-0549           1256 Seamans Center
Sys_admin/Postmaster/cell_admin            Iowa City, IA 52242-1527
#include <std_disclaimer.h>
Better is not better, 'standard' is better. B{

_______________________________________________
Imap-uw mailing list
[email protected]
http://mailman2.u.washington.edu/mailman/listinfo/imap-uw

*** imapd.c.orig	2013-04-16 17:53:36.000000000 -0500
--- imapd.c	2013-06-09 03:06:37.000000000 -0500
***************
*** 32,37 ****
--- 32,38 ----
  #include <signal.h>
  #include <setjmp.h>
  #include <time.h>
+ #include <limits.h>
  #include "c-client.h"
  #include "newsrc.h"
  #include <sys/stat.h>
***************
*** 938,967 ****
  
  				/* wants ALL */
  		if (retval & 0x4) {
! 		  unsigned long j;
  				/* find first match */
  		  for (i = 1; (i <= nmsgs) && !mail_elt (stream,i)->searched;
  		       ++i);
  		  if (i <= nmsgs) {
  		    PSOUT (" ALL ");
! 		    pnum (uid ? mail_uid (stream,i) : i);
! 		    j = i;	/* last message output */
  		  }
! 		  while (++i <= nmsgs) {
! 		    if (mail_elt (stream,i)->searched) {
! 		      while ((++i <= nmsgs) && mail_elt (stream,i)->searched);
! 				/* previous message is end of range */
! 		      if (j != --i) {
! 			PBOUT (':');
! 			pnum (uid ? mail_uid (stream,i) : i);
  		      }
  		    }
  				/* search for next match */
! 		    while ((++i <= nmsgs) && !mail_elt (stream,i)->searched);
! 		    if (i <= nmsgs) {
! 		      PBOUT (',');
! 		      pnum (uid ? mail_uid (stream,i) : i);
! 		      j = i;	/* last message output */
  		    }
  		  }
  		}
--- 939,996 ----
  
  				/* wants ALL */
  		if (retval & 0x4) {
! 		  unsigned long j,juid,iuid;
  				/* find first match */
  		  for (i = 1; (i <= nmsgs) && !mail_elt (stream,i)->searched;
  		       ++i);
  		  if (i <= nmsgs) {
+ 		    juid= (uid ? mail_uid (stream,i) : i);
  		    PSOUT (" ALL ");
! 		    pnum (juid);
! 		    j = i;	/* last message index processed */
  		  }
! 		  if (uid) {	/* doing UIDs, no MRC optimization */
! 		    while (++i <= nmsgs) {
! 		      if (mail_elt (stream,i)->searched) {	/* this message matches */
! 			iuid= mail_uid (stream,i);
! 			if (juid == (iuid - (i-j))) {	/* in sequence of UIDs */
! 			  if ( i < nmsgs) continue;	/* may be more, check next */
! 			  PBOUT (':'); pnum(iuid);	/* done with all, tie it off */
! 			  j = i; juid= iuid;
! 			} else {	/* break in sequence of UIDs */
! 			  if ((i-1) > j) {	/* prev sequence, tie it off */
! 			    j= i-1; juid= mail_uid (stream,j);
! 			    PBOUT (':'); pnum(juid);
! 			  }
! 			  PBOUT (','); pnum(iuid);
! 			  j = i; juid= iuid;	/* record last done message/UID */
! 			}
! 		      } else {	/* this message doesn't match, have a pending sequence? */
! 			/* if pending sequence, tie it off & reset flags */
! 			if ((i-1) > j) {	/* prev sequence, tie it off */
! 			  j= i-1; juid= mail_uid (stream,j);
! 			  PBOUT (':'); pnum(juid);
! 			}
! 			j= nmsgs+1; juid= ULONG_MAX;	/* flag that we're not in a sequence of matches */
  		      }
  		    }
+ 		  } else {	/* doing message-IDs, use MRC optimization */
+ 		    while (++i <= nmsgs) {
+ 		      if (mail_elt (stream,i)->searched) {
+ 		        while ((++i <= nmsgs) && mail_elt (stream,i)->searched);
+ 				/* previous message is end of range */
+ 		        if (j != --i) {
+ 	 		  PBOUT (':');
+ 			  pnum (i);
+ 		        }
+ 		    }
  				/* search for next match */
! 		      while ((++i <= nmsgs) && !mail_elt (stream,i)->searched);
! 		      if (i <= nmsgs) {
! 		        PBOUT (',');
! 		        pnum (i);
! 		        j = i;	/* last message output */
! 		      }
  		    }
  		  }
  		}