Menu
▾
▴
fetchmail-devel
|
From: Sunil S. <sh...@bo...> - 2010-01-28 09:52:05
|
Hi Matthias, I am thinking of making some improvements to imap search in fetchmail. Please check which of these improvements are acceptable for fetchmail 6.3 series. ============================================================================================ 1. In r3838, the "SEARCH UNSEEN" was changed to "SEARCH UNSEEN NOT DELETED". Some imap servers do not support such a complex search phrase. I could find only one reference for that: http://lists.ccil.org/pipermail/fetchmail-friends/2004-June/008897.html I propose that that change should be undone. Note that "SEARCH UNSEEN NOT DELETED" gives different result from "SEARCH UNSEEN" only when - another e-mail client has marked some mails for deletion without seeing, - that e-mail client has not expunged the deleted mails, and - fetchmail is keeping mails (so that fetchmail also has not expunged the deleted mails). The comment in the code reads: /* don't count deleted messages, in case user enabled keep last time */ This comment is incorrect. This is because fetchmail always marks the mail as \Seen irrespective of whether fetchmail deletes it or keeps it. So, that mail will not show up in the next "SEARCH UNSEEN". ============================================================================================ 2. Some servers do not support "SEARCH" at all. Here are some references for that: http://lists.ccil.org/pipermail/fetchmail-friends/2004-May/008675.html http://lists.ccil.org/pipermail/fetchmail-friends/2005-January/009351.html In this case, as a fallback, fetchmail should use something like: FETCH 1:1000 FLAGS and process all mails without \Seen as unseen. ============================================================================================ 3. The reply to "SEARCH UNSEEN" overflows fetchmail buffer when there are more than 1860 unseen mails. fetchmail should do a range search: SEARCH 1:1000 UNSEEN SEARCH 1001:2000 UNSEEN ... ============================================================================================ -- Sunil Shetye. |
|
From: Matthias A. <mat...@gm...> - 2010-02-01 20:29:14
|
Am 28.01.2010 10:04, schrieb Sunil Shetye: > I am thinking of making some improvements to imap search in fetchmail. > Please check which of these improvements are acceptable for fetchmail > 6.3 series. Dear Sunil, great! All proposed changes are acceptable, with comments below, and I definitely trust your code. All past contributions that passed through my hands were of excellent quality, so thanks a bunch! > ============================================================================================ > 1. In r3838, the "SEARCH UNSEEN" was changed to "SEARCH UNSEEN NOT > DELETED". Some imap servers do not support such a complex search > phrase. I could find only one reference for that: > > http://lists.ccil.org/pipermail/fetchmail-friends/2004-June/008897.html > > I propose that that change should be undone. > > Note that "SEARCH UNSEEN NOT DELETED" gives different result from > "SEARCH UNSEEN" only when > > - another e-mail client has marked some mails for deletion without > seeing, > - that e-mail client has not expunged the deleted mails, and > - fetchmail is keeping mails (so that fetchmail also has not expunged > the deleted mails). > > The comment in the code reads: > > /* don't count deleted messages, in case user enabled keep last time */ > > This comment is incorrect. This is because fetchmail always marks the > mail as \Seen irrespective of whether fetchmail deletes it or keeps > it. So, that mail will not show up in the next "SEARCH UNSEEN". Fixing comments is appreciated and always acceptable for supported versions. The issue reported 5 years ago refers to a regression on IMAP2 servers, and a regression fix is also always acceptable (though 6.2.X remains unsupported by me, I'm not releasing another 6.2.X - the fix will go into 6.3.X then). In the implementation of the fix, could you switch by protocol version to use the "... NOT DELETED" form for newer servers (IMAP4r1, not sure if IMAP4 also supports it), and the older "SEARCH UNSEEN" only in IMAP2? That would give us the best of both approaches -- or perhaps we could just fall back to SEARCH UNSEEN if SEARCH UNSEEN NOT DELETED fails? > ============================================================================================ > 2. Some servers do not support "SEARCH" at all. Here are some > references for that: > > http://lists.ccil.org/pipermail/fetchmail-friends/2004-May/008675.html > http://lists.ccil.org/pipermail/fetchmail-friends/2005-January/009351.html > > In this case, as a fallback, fetchmail should use something like: > > FETCH 1:1000 FLAGS > > and process all mails without \Seen as unseen. Much appreciated. OK since it extends fetchmail's audience. > ============================================================================================ > 3. The reply to "SEARCH UNSEEN" overflows fetchmail buffer when there > are more than 1860 unseen mails. fetchmail should do a range search: > > SEARCH 1:1000 UNSEEN > SEARCH 1001:2000 UNSEEN > ... Desirable bug fix. Hope that helps. Looking forward to your patches! Best regards Matthias |
|
From: Sunil S. <sh...@bo...> - 2010-02-03 13:15:41
Attachments:
fetchmail-6.3-imapsearch.patch
|
Hi Matthias, Please check the attached patch for improving imap search. Quoting from Matthias Andree's mail on Mon, Feb 01, 2010: > > 1. In r3838, the "SEARCH UNSEEN" was changed to "SEARCH UNSEEN NOT > > DELETED". Some imap servers do not support such a complex search > > phrase. I could find only one reference for that: > > > > http://lists.ccil.org/pipermail/fetchmail-friends/2004-June/008897.html > > > > I propose that that change should be undone. ... > The issue reported 5 years ago refers to a regression on IMAP2 servers, and a > regression fix is also always acceptable (though 6.2.X remains unsupported by > me, I'm not releasing another 6.2.X - the fix will go into 6.3.X then). > > In the implementation of the fix, could you switch by protocol version to use > the "... NOT DELETED" form for newer servers (IMAP4r1, not sure if IMAP4 also > supports it), and the older "SEARCH UNSEEN" only in IMAP2? That would give us > the best of both approaches -- or perhaps we could just fall back to SEARCH > UNSEEN if SEARCH UNSEEN NOT DELETED fails? Ok, I have ensured that "SEARCH UNSEEN" is used for IMAP2 servers. Also, I have also changed "NOT DELETED" to "UNDELETED" assuming that "SEARCH UNSEEN UNDELETED" will be faster than "SEARCH UNSEEN NOT DELETED". This syntax is acceptable as per IMAP4 and IMAP4r1 RFCs. > > 2. Some servers do not support "SEARCH" at all. Here are some > > references for that: > > > > http://lists.ccil.org/pipermail/fetchmail-friends/2004-May/008675.html > > http://lists.ccil.org/pipermail/fetchmail-friends/2005-January/009351.html > > > > In this case, as a fallback, fetchmail should use something like: > > > > FETCH 1:1000 FLAGS > > > > and process all mails without \Seen as unseen. > > Much appreciated. OK since it extends fetchmail's audience. ... > > 3. The reply to "SEARCH UNSEEN" overflows fetchmail buffer when there > > are more than 1860 unseen mails. fetchmail should do a range search: > > > > SEARCH 1:1000 UNSEEN > > SEARCH 1001:2000 UNSEEN > > ... > > Desirable bug fix. ... > Looking forward to your patches! Thanks a lot for that. I am submitting only one patch to fix all the mentioned issues. It is not practical for me to submit separate patches since all the issues are interlinked. Hope that that is acceptable to you. Here are the expected fallbacks this patch will do. This means that if one search command fails, this patch will use the next command as per the following list: Case 1) no fetchall keep IMAP4 server or higher 2000 mails in mailbox IMAP> SEARCH 1:1000 UNSEEN UNDELETED IMAP> SEARCH 1:1000 UNSEEN # fallback 1 IMAP> SEARCH UNSEEN # fallback 2 IMAP> FETCH 1:2000 FLAGS # fallback 3 Case 2) no fetchall keep IMAP4 server or higher 300 mails in mailbox IMAP> SEARCH UNSEEN UNDELETED IMAP> SEARCH UNSEEN # fallback 1 IMAP> FETCH 1:300 FLAGS # fallback 2 Case 3) no fetchall no keep or IMAP2 server or both 4000 mails in mailbox IMAP> SEARCH 1:1000 UNSEEN IMAP> SEARCH UNSEEN # fallback 1 IMAP> FETCH 1:4000 FLAGS # fallback 2 Case 4) no fetchall no keep or IMAP2 server or both 500 mails in mailbox IMAP> SEARCH UNSEEN IMAP> FETCH 1:500 FLAGS # fallback 1 Also, is there an IMAP2 server available which can be used for testing this patch? -- Sunil Shetye. |
|
From: Matthias A. <mat...@gm...> - 2010-02-04 10:56:20
|
Sunil Shetye schrieb am 2010-02-03:
> Hi Matthias,
>
> Please check the attached patch for improving imap search.
Good, applied as rev 5468.
Note I've added a patch on top of it - please check if that's OK to you.
Note also that I'll likely be releasing 6.3.14 tonight to fix a security
issue in verbose SSL/TLS certificate display on platforms where char is
signed. The paranoid and impatient may want to recompile fetchmail with
"CC=gcc -funsigned-char" for the nonce.
commit 63587e34de582abe3b4a611437fc2fce8b5bd81e
Author: m-a <m-a@6bd12b38-53dc-0310-98db-d94f1ca4f90c>
Date: Thu Feb 4 09:51:08 2010 +0000
Stricter validation of IMAP responses containing byte or message counts.
git-svn-id: svn+ssh://mknod.org/svn/fetchmail/branches/BRANCH_6-3@5469 6bd12b38-53dc-0310-98db-d94f1ca4f90c
diff --git a/NEWS b/NEWS
index 4b64b64..565d7bd 100644
--- a/NEWS
+++ b/NEWS
@@ -79,6 +79,7 @@ fetchmail 6.3.14 (not yet released):
Note that this wasn't security relevant because fetchmail would only read up
to the maximum buffer size and leave the remainder of the string unread, going
out of synch afterwards.
+* Stricter validation of IMAP responses containing byte or message counts.
# CHANGES
* Only include gssapi.h if we're not including gssapi/gssapi.h, to fix a FreeBSD
diff --git a/imap.c b/imap.c
index 89d486c..15a563e 100644
--- a/imap.c
+++ b/imap.c
@@ -74,7 +74,9 @@ static int imap_untagged_response(int sock, const char *buf)
}
else if (strstr(buf, " EXISTS"))
{
- count = atoi(buf+2);
+ char *t; unsigned long u;
+ errno = 0;
+ u = strtoul(buf+2, &t, 10);
/*
* Don't trust the message count passed by the server.
* Without this check, it might be possible to do a
@@ -82,11 +84,15 @@ static int imap_untagged_response(int sock, const char *buf)
* count, and allocate a malloc area that would overlap
* a portion of the stack.
*/
- if ((unsigned)count > INT_MAX/sizeof(int))
+ if (errno /* strtoul failed */
+ || t == buf+2 /* no valid data */
+ || u > (unsigned long)(INT_MAX/sizeof(int)) /* too large */)
{
- report(stderr, GT_("bogus message count!"));
+ report(stderr, GT_("bogus message count in \"%s\"!"), buf);
return(PS_PROTOCOL);
}
+ count = u; /* safe as long as count <= INT_MAX - checked above */
+
if ((recentcount = count - oldcount) < 0)
recentcount = 0;
@@ -117,14 +123,22 @@ static int imap_untagged_response(int sock, const char *buf)
# if 0
else if (strstr(buf, " RECENT"))
{
+ /* fixme: use strto[u]l and error checking */
recentcount = atoi(buf+2);
}
# endif
else if (strstr(buf, " EXPUNGE"))
{
+ unsigned long u; char *t;
/* the response "* 10 EXPUNGE" means that the currently
* tenth (i.e. only one) message has been deleted */
- if (atoi(buf+2) > 0)
+ errno = 0;
+ u = strtoul(buf+2, &t, 10);
+ if (errno /* conversion error */ || t == buf+2 /* no number found */) {
+ report(stderr, GT_("bogus EXPUNGE count in \"%s\"!"), buf);
+ return PS_PROTOCOL;
+ }
+ if (u > 0)
{
if (count > 0)
count--;
@@ -854,7 +868,8 @@ restartsearch:
errno = 0;
um = strtoul(cp,&ep,10);
- if (errno == 0 && um <= UINT_MAX && um <= (unsigned)count)
+ if (errno == 0 && ep > cp
+ && um <= INT_MAX && um <= (unsigned)count)
{
unseen_messages[unseen++] = um;
if (outlevel >= O_DEBUG)
@@ -912,7 +927,7 @@ fetchflags:
*/
if (unseen < count
&& sscanf(buf, "* %u FETCH ", &num) == 1
- && num >= 1 && num <= count
+ && num >= 1 && num <= (unsigned)count
&& strstr(buf, "FLAGS ")
&& !strstr(buf, "\\SEEN")
&& !strstr(buf, "\\DELETED"))
@@ -1302,15 +1317,21 @@ static int imap_fetch_body(int sock, struct query *ctl, int number, int *lenp)
* server called dbmail that returns huge garbage lengths.
*/
if ((cp = strchr(buf, '{'))) {
+ long l; char *t;
errno = 0;
- *lenp = (int)strtol(cp + 1, (char **)NULL, 10);
- if (errno == ERANGE || *lenp < 0)
- *lenp = -1; /* length is too big/small for us to handle */
- }
- else
+ ++ cp;
+ l = strtol(cp, &t, 10);
+ if (errno || t == cp || (t && !strchr(t, '}')) /* parse error */
+ || l < 0 || l > INT_MAX /* range check */) {
+ *lenp = -1;
+ } else {
+ *lenp = l;
+ }
+ } else {
*lenp = -1; /* missing length part in FETCH reponse */
+ }
- return(PS_SUCCESS);
+ return PS_SUCCESS;
}
static int imap_trail(int sock, struct query *ctl, const char *tag)
|
|
From: Sunil S. <sh...@bo...> - 2010-02-04 13:14:28
|
Quoting from Matthias Andree's mail on Thu, Feb 04, 2010: > > Please check the attached patch for improving imap search. > > Good, applied as rev 5468. > > Note I've added a patch on top of it - please check if that's OK to you. Yeah, that looks okay. > Note also that I'll likely be releasing 6.3.14 tonight to fix a security > issue in verbose SSL/TLS certificate display on platforms where char is > signed. The paranoid and impatient may want to recompile fetchmail with > "CC=gcc -funsigned-char" for the nonce. Great. -- Sunil Shetye. |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X