Bug 88850

Summary: [patch] mail/fetchmail misses some messages since patch-imap.c
Product: Ports & Packages Reporter: Vasil Dimov <vd>
Component: Individual Port(s)Assignee: Simon Barner <barner>
Status: Closed FIXED    
Severity: Affects Only Me CC: matthias.andree
Priority: Normal    
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
fetchmail_count.diff none

Description Vasil Dimov 2005-11-11 17:20:15 UTC 
Since files/patch-imap.c appeared fetchmail (sometimes) leaves one
message on the server.

Here is dump of one erroneous session with packets from server dumped
to stdout. There is one message on the server, but fetchmail says
"No mail".

IN >>> buf=* OK READY.
IN >>> buf=* CAPABILITY IMAP4REV1 SORT THREAD=REFERENCES MULTIAPPEND UNSELECT LITERAL+ IDLE CHILDREN NAMESPACE LOGIN-REFERRALS AUTH=PLAIN
IN >>> buf=A0001 OK CAPABILITY COMPLETED.
IN >>> buf=A0002 OK LOGGED IN.
IN >>> buf=* FLAGS (\ANSWERED \FLAGGED \DELETED \SEEN \DRAFT)
IN >>> buf=* OK [PERMANENTFLAGS (\ANSWERED \FLAGGED \DELETED \SEEN \DRAFT \*)] FLAGS PERMITTED.
IN >>> buf=* 1 EXISTS
IN >>> buf=* 0 RECENT
IN >>> buf=* OK [UIDVALIDITY 1130238900] UIDS VALID
IN >>> buf=* OK [UIDNEXT 166] PREDICTED NEXT UID
IN >>> buf=A0003 OK [READ-WRITE] SELECT COMPLETED.
IN >>> buf=A0004 OK EXPUNGE COMPLETED.
decreasing count, buf+2=004 OK EXPUNGE COMPLETED.
fetchmail: No mail for vd at localhost (folder freebsd-amd64)
IN >>> buf=* BYE LOGGING OUT
IN >>> buf=A0005 OK LOGOUT COMPLETED.

Obviously buf+2 is in the middle of the fetchmail's tag, echoed by the
server, so atoi(buf+2) gives 4 in this case.

Is this the intended operation?

Maybe untagged command is expected here, eg "* 1 EXPUNGE"?
If this is true we should better go for the string after the first
space, instead of cutting the first two chars.

Fix: Be careful, I do not fully understand the meaning of the code I patched.
Comment 1 Edwin Groothuis freebsd_committer freebsd_triage 2005-11-11 17:27:18 UTC 
Responsible Changed
From-To: freebsd-ports-bugs->barner

Over to maintainer
Comment 2 Vasil Dimov 2005-11-14 10:23:23 UTC 
On Sat, Nov 12, 2005 at 06:02:56PM +0100, Matthias Andree wrote:
> Please replace the broken patch by this one - yes, it's bulky...
> 
> (I've attached the file, so you can pick whatever is more convenient for
> you).
> 
> Index: imap.c
> ===================================================================
> --- imap.c	(revision 4423)
> +++ imap.c	(working copy)
> @@ -57,72 +57,85 @@
>  		*cp = toupper(*cp);
>  
>  	/* interpret untagged status responses */
> -	if (strstr(buf, "* CAPABILITY"))
> -	{
> -	    strncpy(capabilities, buf + 12, sizeof(capabilities));
> -	    capabilities[sizeof(capabilities)-1] = '\0';
> -	}
> -	else if (strstr(buf, "EXISTS"))
> -	{
> -	    count = atoi(buf+2);
> -	    /*
> -	     * Don't trust the message count passed by the server.
> -	     * Without this check, it might be possible to do a
> -	     * DNS-spoofing attack that would pass back a ridiculous 
> -	     * count, and allocate a malloc area that would overlap
> -	     * a portion of the stack.
> -	     */
> -	    if (count > INT_MAX/sizeof(int))
> +	if (buf[0] == '*' && buf[1] == ' ') {
> +	    if (strstr(buf, " CAPABILITY"))
>  	    {
> -		report(stderr, "bogus message count!");
> -		return(PS_PROTOCOL);
> +		strncpy(capabilities, buf + 12, sizeof(capabilities));
> +		capabilities[sizeof(capabilities)-1] = '\0';
>  	    }
> -
> -	    /*
> -	     * Nasty kluge to handle RFC2177 IDLE.  If we know we're idling
> -	     * we can't wait for the tag matching the IDLE; we have to tell the
> -	     * server the IDLE is finished by shipping back a DONE when we
> -	     * see an EXISTS.  Only after that will a tagged response be
> -	     * shipped.  The idling flag also gets cleared on a timeout.
> -	     */
> -	    if (stage == STAGE_IDLE)
> +	    else if (strstr(buf, " EXISTS"))
>  	    {
> -		/* If IDLE isn't supported, we were only sending NOOPs anyway. */
> -		if (has_idle)
> +		count = atoi(buf+2);
> +		/*
> +		 * Don't trust the message count passed by the server.
> +		 * Without this check, it might be possible to do a
> +		 * DNS-spoofing attack that would pass back a ridiculous 
> +		 * count, and allocate a malloc area that would overlap
> +		 * a portion of the stack.
> +		 */
> +		if (count > INT_MAX/sizeof(int))
>  		{
> -		    /* we do our own write and report here to disable tagging */
> -		    SockWrite(sock, "DONE\r\n", 6);
> -		    if (outlevel >= O_MONITOR)
> -		        report(stdout, "IMAP> DONE\n");
> +		    report(stderr, "bogus message count!");
> +		    return(PS_PROTOCOL);
>  		}
>  
> -		mytimeout = saved_timeout;
> -		stage = STAGE_FETCH;
> +		/*
> +		 * Nasty kluge to handle RFC2177 IDLE.  If we know we're idling
> +		 * we can't wait for the tag matching the IDLE; we have to tell the
> +		 * server the IDLE is finished by shipping back a DONE when we
> +		 * see an EXISTS.  Only after that will a tagged response be
> +		 * shipped.  The idling flag also gets cleared on a timeout.
> +		 */
> +		if (stage == STAGE_IDLE)
> +		{
> +		    /* If IDLE isn't supported, we were only sending NOOPs anyway. */
> +		    if (has_idle)
> +		    {
> +			/* we do our own write and report here to disable tagging */
> +			SockWrite(sock, "DONE\r\n", 6);
> +			if (outlevel >= O_MONITOR)
> +			    report(stdout, "IMAP> DONE\n");
> +		    }
> +
> +		    mytimeout = saved_timeout;
> +		    stage = STAGE_FETCH;
> +		}
>  	    }
> +	    /* a space is required to avoid confusion with the \Recent flag */
> +	    else if (strstr(buf, " RECENT"))
> +	    {
> +		recentcount = atoi(buf+2);
> +	    }
> +	    /* servers are not required to update the EXISTS counts,
> +	     * so count down by ourselves */
> +	    else if (strstr(buf, " EXPUNGE"))
> +	    {
> +		if (atoi(buf+2) > 0) {
> +		    if (count > 0)
> +			count --;
> +		}
> +	    }
> +	    else if (strstr(buf, " PREAUTH")) {
> +		preauth = TRUE;
> +	    }
> +	    /*
> +	     * The server may decide to make the mailbox read-only, 
> +	     * which causes fetchmail to go into a endless loop
> +	     * fetching the same message over and over again. 
> +	     * 
> +	     * However, for check_only, we use EXAMINE which will
> +	     * mark the mailbox read-only as per the RFC.
> +	     * 
> +	     * This checks for the condition and aborts if 
> +	     * the mailbox is read-only. 
> +	     *
> +	     * See RFC 2060 section 6.3.1 (SELECT).
> +	     * See RFC 2060 section 6.3.2 (EXAMINE).
> +	     */ 
> +	    else if (!check_only && strstr(buf, "[READ-ONLY]")) {
> +		return(PS_LOCKBUSY);
> +	    }
>  	}
> -	/* a space is required to avoid confusion with the \Recent flag */
> -	else if (strstr(buf, " RECENT"))
> -	{
> -	    recentcount = atoi(buf+2);
> -	}
> -	else if (strstr(buf, "PREAUTH"))
> -	    preauth = TRUE;
> -	/*
> -	 * The server may decide to make the mailbox read-only, 
> -	 * which causes fetchmail to go into a endless loop
> -	 * fetching the same message over and over again. 
> -	 * 
> -	 * However, for check_only, we use EXAMINE which will
> -	 * mark the mailbox read-only as per the RFC.
> -	 * 
> -	 * This checks for the condition and aborts if 
> -	 * the mailbox is read-only. 
> -	 *
> -	 * See RFC 2060 section 6.3.1 (SELECT).
> -	 * See RFC 2060 section 6.3.2 (EXAMINE).
> -	 */ 
> -	else if (!check_only && strstr(buf, "[READ-ONLY]"))
> -	    return(PS_LOCKBUSY);
>      } while
>  	(tag[0] != '\0' && strncmp(buf, tag, strlen(tag)));
>  
> 
> -- 
> Matthias Andree


With this patch fetchmail does not leave messages on the server,
that is - it fixes the problem I reported. Thanks.

-- 
Vasil Dimov
Comment 3 Simon Barner freebsd_committer freebsd_triage 2005-11-14 11:49:13 UTC 
State Changed
From-To: open->closed

Patch committed, thanks!