Menu
▾
▴
fetchmail-devel
|
From: Manfred W. <we...@ic...> - 2005-03-05 23:16:14
|
I have a problem with fetchmail, which occurs every once in a while (and
also happened with older versions of fetchmail). I use fetchmail in
daemon mode (started from my KDE startup script when I log in) to poll
two mail servers.
Sometimes when I log in the .fetchids file is very short, it contains
only very few uids. And therefore fetchmail loads down very many old
mails. And since I get very much spam, this is very much, often several
thousand mails. My first thought was, that probably fetchmail is killed
in a bad moment during writing the .fetchids by system shutdown. This is
one possibility, but I had a look into the code and found a bug, which
also could cause this problem.
In the function uid_swap_lists (in uid.c) there is this code:
if (ctl->newsaved)
{
/* old state of mailbox may now be irrelevant */
if (outlevel >= O_DEBUG)
report(stdout, GT_("swapping UID lists\n"));
free_str_list(&ctl->oldsaved);
ctl->oldsaved = ctl->newsaved;
ctl->newsaved = (struct idlist *) NULL;
}
So first the old oldsaved list is freed, and then it is overwritten with
the newsaved list. If the TERM signal arrives during the free_str_list,
an incomplete oldsaved list will be saved to .fetchids. And since
free_str_list is implemented by a recursion, it really shortens the list
beginning at the end (BTW I do not know, why this is implemented by a
recursion rather then by a loop. If there are many messages on the
server this could lead to a really deep recursion with the possibility
of a stack overflow).
IMHO the correct solution would be:
if (ctl->newsaved)
{
struct idlist *temp;
/* old state of mailbox may now be irrelevant */
if (outlevel >= O_DEBUG)
report(stdout, GT_("swapping UID lists\n"));
temp = ctrl->oldsaved;
ctl->oldsaved = ctl->newsaved;
ctl->newsaved = (struct idlist *) NULL;
free_str_list(&ctl->oldsaved);
}
An additional good idea would be not to overwrite the old idfile, but
write to new idfile (e.g. ~/.fetchids-new), and after completion rename
the file (e.g. "rename("~/.fetchids-new", "~/.fetchids");" or similar).
Since there is a comment in the code "FIXME: do not overwrite the old
idfile" I assume that you already planned such change and I wonder, why
you did not implement it. I think, it is a very easy change, it is
probably just about 3 or 4 additional lines; if you want, I can provide
you a patch.
Here is the information you require (according to the FAQ) in a bug report:
1. Linux 2.6.10 (but the problem occurred also with older versions)
2. fetchmail_6.2.5-12_i386.deb
3. I do not see a greeting when logging into the pop3 server (not
relevant for the problem anyway).
4. Exim 4.34
5. -d 600
5.
This is fetchmail release 6.2.5+NTLM+SDPS+SSL+NLS
Fallback MDA: (none)
Linux pc235 2.6.10 #1 Mon Dec 27 10:43:40 CET 2004 i686 GNU/Linux
Taking options from command line and /home/weihs/.fetchmailrc
Idfile is /home/weihs/.fetchids
Fetchmail will show progress dots even in logfiles.
Fetchmail will forward misaddressed multidrop messages to weihs.
Options for retrieving from XX...@XX....XX:
True name of server is mail.ict.tuwien.ac.at.
Protocol is POP3 (forcing UIDL use).
All available authentication methods will be tried.
SSL encrypted sessions enabled.
Server nonresponse timeout is 300 seconds (default).
Default mailbox selected.
Only new messages will be retrieved (--all off).
Fetched messages will be kept on the server (--keep on).
Old messages will not be flushed before message retrieval (--flush off).
Rewrite of server-local addresses is enabled (--norewrite off).
Carriage-return stripping is disabled (stripcr off).
Carriage-return forcing is disabled (forcecr off).
Interpretation of Content-Transfer-Encoding is enabled (pass8bits off).
MIME decoding is disabled (mimedecode off).
Idle after poll is disabled (idle off).
Nonempty Status lines will be kept (dropstatus off)
Delivered-To lines will be kept (dropdelivered off)
Fetch message size limit is 100 (--fetchsizelimit 100).
Do binary search of UIDs during 9 out of 10 polls (--fastuidl 10).
Messages will be SMTP-forwarded to: localhost (default)
Single-drop mode: 1 local name(s) recognized.
4098 UIDs saved.
Manfred Weihs
|
|
From: Matthias A. <mat...@gm...> - 2005-03-06 02:25:35
|
Manfred Weihs <we...@ic...> writes:
> I have a problem with fetchmail, which occurs every once in a while (and
> also happened with older versions of fetchmail). I use fetchmail in
> daemon mode (started from my KDE startup script when I log in) to poll
> two mail servers.
I'll just copy part of the message I've sent to fetchmail-friends, for
completeness's sake:
-------------------------------------------------------------------------
Manfred Weihs <we...@ic...> writes:
> Sometimes when I log in the .fetchids file is very short, it contains
> only very few uids. And therefore fetchmail loads down very many old
> mails. And since I get very much spam, this is very much, often several
> thousand mails.
Perhaps keeping all spam mail on the server isn't the best idea then?
> My first thought was, that probably fetchmail is killed
> in a bad moment during writing the .fetchids by system shutdown. This is
> one possibility, but I had a look into the code and found a bug, which
> also could cause this problem.
>
> In the function uid_swap_lists (in uid.c) there is this code:
> if (ctl->newsaved)
> {
> /* old state of mailbox may now be irrelevant */
> if (outlevel >= O_DEBUG)
> report(stdout, GT_("swapping UID lists\n"));
> free_str_list(&ctl->oldsaved);
> ctl->oldsaved = ctl->newsaved;
> ctl->newsaved = (struct idlist *) NULL;
> }
>
> So first the old oldsaved list is freed, and then it is overwritten with
> the newsaved list. If the TERM signal arrives during the free_str_list,
> an incomplete oldsaved list will be saved to .fetchids. And since
> free_str_list is implemented by a recursion, it really shortens the list
> beginning at the end (BTW I do not know, why this is implemented by a
> recursion rather then by a loop. If there are many messages on the
> server this could lead to a really deep recursion with the possibility
> of a stack overflow).
This is one of the many flaws in existing UID code. The problem is that
the current maintainer team including backup maintainers has evidently
zero time. A handover from ESR was started half a year ago, but
effectively starved in action; Rob Funk has essentially dropped from the
net as ESR did before, and Graham Wilson has officially quit but is
still operating the SVN server.
If you'd be willing to help, that would be much appreciated.
> IMHO the correct solution would be:
> if (ctl->newsaved)
> {
> struct idlist *temp;
> /* old state of mailbox may now be irrelevant */
> if (outlevel >= O_DEBUG)
> report(stdout, GT_("swapping UID lists\n"));
> temp = ctrl->oldsaved;
> ctl->oldsaved = ctl->newsaved;
> ctl->newsaved = (struct idlist *) NULL;
> free_str_list(&ctl->oldsaved);
> }
Actually, the right code is:
if (ctl->newsaved)
{
/* old state of mailbox may now be irrelevant */
struct idlist **temp = &ctl->oldsaved;
if (outlevel >= O_DEBUG)
report(stdout, GT_("swapping UID lists\n"));
ctl->oldsaved = ctl->newsaved;
ctl->newsaved = (struct idlist *) NULL;
free_str_list(temp);
}
> An additional good idea would be not to overwrite the old idfile, but
> write to new idfile (e.g. ~/.fetchids-new), and after completion rename
> the file (e.g. "rename("~/.fetchids-new", "~/.fetchids");" or similar).
> Since there is a comment in the code "FIXME: do not overwrite the old
> idfile" I assume that you already planned such change and I wonder, why
> you did not implement it. I think, it is a very easy change, it is
> probably just about 3 or 4 additional lines; if you want, I can provide
> you a patch.
Well, we have code from Sunil Shetye, another "backup maintainer" who
disappeared without notice, to use one .fetchids file per server, and
that might (I'd need to check again) already implement that, so the
immediate fix may have been shelved for that reason. The patch was
queued for 6.2.7 as 6.2.6 was supposed to be bugfix-only.
I have made the fix of writing to a temp file first, as this has been
long-standing and is not too intrusive. (SVN revision 4019)
--
Matthias Andree
|
|
From: Matthias A. <mat...@gm...> - 2005-03-06 03:00:32
|
Matthias Andree <mat...@gm...> writes:
> Actually, the right code is:
>
> if (ctl->newsaved)
> {
> /* old state of mailbox may now be irrelevant */
> struct idlist **temp = &ctl->oldsaved;
> if (outlevel >= O_DEBUG)
> report(stdout, GT_("swapping UID lists\n"));
> ctl->oldsaved = ctl->newsaved;
> ctl->newsaved = (struct idlist *) NULL;
> free_str_list(temp);
> }
Of course it's not, we'll be erasing everything.
Right code is (watch where the & is!)
if (ctl->newsaved)
{
/* old state of mailbox may now be irrelevant */
struct idlist *temp = ctl->oldsaved;
if (outlevel >= O_DEBUG)
report(stdout, GT_("swapping UID lists\n"));
ctl->oldsaved = ctl->newsaved;
ctl->newsaved = (struct idlist *) NULL;
free_str_list(&temp);
}
Committed as SVN revision 4021.
--
Matthias Andree
|
|
From: Manfred W. <we...@ic...> - 2005-03-06 15:15:40
Attachments:
fetchmail.diff
|
Matthias Andree wrote:
> Of course it's not, we'll be erasing everything.
>
> Right code is (watch where the & is!)
>
> if (ctl->newsaved)
> {
> /* old state of mailbox may now be irrelevant */
> struct idlist *temp = ctl->oldsaved;
> if (outlevel >= O_DEBUG)
> report(stdout, GT_("swapping UID lists\n"));
> ctl->oldsaved = ctl->newsaved;
> ctl->newsaved = (struct idlist *) NULL;
> free_str_list(&temp);
> }
>
> Committed as SVN revision 4021.
OK, this is now definitely the correct version. Sorry for the small bug
in my first version, and thanks for committing the two fixes to svn. I
think this should solve my problems.
You might have a look at the attached patch. It removes the (completely
unnecessary) recursion in free_str_list. So it reduces stack usage and
avoids possible problems with very long lists. Furthermore I slightly
changed the interface and replaced the pointer to a pointer to the first
list element by just a pointer to the first list element. I think, this
is cleaner; the only difference is that it is now in some cases
necessary to set the pointer to NULL afterwards, but this is already
done at many locations anyway.
> Perhaps keeping all spam mail on the server isn't the best idea then?
But since I fetch the mails from two PCs (at home and at work), I have
to leave them on the server. Maybe I should delete mails from the server
more frequently. But I have to do this manually, because fetchmail does
not support to keep the mails only for X days or to keep only the latest
X mails (the later would be very easy to implement, but according to the
fetchmail FAQ it will not be implemented due to political reasons). And
an automatic cron job is no alternative, because, in this case I cannot
be sure, that fetchmail got all the mails before the cron job deletes
them. The deletion should be done _after_ fetchmail delivered the mails
locally, ideally within the same pop3 session.
Manfred Weihs
|
|
From: Matthias A. <mat...@gm...> - 2005-03-06 16:33:28
|
Manfred Weihs schrieb am 2005-03-06: > You might have a look at the attached patch. It removes the (completely > unnecessary) recursion in free_str_list. So it reduces stack usage and OK that far. However... > avoids possible problems with very long lists. Furthermore I slightly > changed the interface and replaced the pointer to a pointer to the first > list element by just a pointer to the first list element. I think, this ...now is not the time for interface changes, as the next fetchmail release has been long overdue and intrusive changes should wait. So I've left the interface unchanged and just removed the recursion. I have ignored your change to the __UNUSED__ code which isn't compiled in. > > Perhaps keeping all spam mail on the server isn't the best idea then? > > But since I fetch the mails from two PCs (at home and at work), I have > to leave them on the server. Maybe I should delete mails from the server > more frequently. But I have to do this manually, because fetchmail does > not support to keep the mails only for X days or to keep only the latest > X mails (the later would be very easy to implement, but according to the > fetchmail FAQ it will not be implemented due to political reasons). Yup, that was ESR's opinion. I don't share this view, but the UID code needs to be cleaned up *massively* before any feature changes should be allowed in. The UID code is a mess, and it's not used for IMAP. > And an automatic cron job is no alternative, because, in this case I > cannot be sure, that fetchmail got all the mails before the cron job > deletes them. The deletion should be done _after_ fetchmail delivered > the mails locally, ideally within the same pop3 session. Right. If you can live with a slower software that has less features in several areas, Charles Cazabon's getmail-4 has this feature. -- Matthias Andree |
|
From: Manfred W. <we...@ic...> - 2005-03-06 17:04:24
|
Matthias Andree wrote: > Manfred Weihs schrieb am 2005-03-06: >>avoids possible problems with very long lists. Furthermore I slightly >>changed the interface and replaced the pointer to a pointer to the first >>list element by just a pointer to the first list element. I think, this > > > ...now is not the time for interface changes, as the next fetchmail > release has been long overdue and intrusive changes should wait. OK. I did not regard the change very intrusive and thought removing one level of dereferencing would make the code cleaner, but it is fine, as you commited it. > Right. If you can live with a slower software that has less features in > several areas, Charles Cazabon's getmail-4 has this feature. Thanks for the hint. The last time I had a look at getmail there was some missing feature that prevented me from using it. But maybe it improved meanwhile. I will have a look at it. Manfred Weihs |
|
From: Brian C. <B.C...@po...> - 2005-03-06 16:46:10
|
On Sun, Mar 06, 2005 at 03:15:36PM +0100, Manfred Weihs wrote: > > Perhaps keeping all spam mail on the server isn't the best idea then? > > But since I fetch the mails from two PCs (at home and at work), I have > to leave them on the server. This was a problem for me, downloading to laptop on the go and desktop where I kept everything archived. I finally solved it by downloading to whichever I happened to be on, keeping all my messages in Maildir format (so each message is in a separate file), and then using 'unison' to synchronise the two home directories from time to time. Works beautifully, and also means my laptop is fully backed up to my desktop PC, and vice versa. Regards, Brian. |
×
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