Re: [fetchmail-devel] lost UIDs

[Matthias A. <mat...@gm...>]

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