fetchmail-devel

[fetchmail-devel] RFC: Patch to fix Berlios Bug #5849

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

Greetings,

Joachim Feise reported that his smtphost setting is lost/ignored since
6.3.0. I could reproduce this.
<http://developer.berlios.de/bugs/?func=detailbug&bug_id=5849&group_id=1824>

However, I use neither LMTP nor multidrop, so I need help determining if
the attached patch that I made is safe.

Looking forward to your reviews.

-- 
Matthias Andree

Re: [fetchmail-devel] RFC: Patch to fix Berlios Bug #5849

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

Matthias Andree <mat...@gm...> writes:

> Greetings,
>
> Joachim Feise reported that his smtphost setting is lost/ignored since
> 6.3.0. I could reproduce this.
> <http://developer.berlios.de/bugs/?func=detailbug&bug_id=5849&group_id=1824>
>
> However, I use neither LMTP nor multidrop, so I need help determining if
> the attached patch that I made is safe.

Hm. This is the same sink.c patch but without space changes and should
thus be easier to review. Please comment:


Index: sink.c
===================================================================
--- sink.c	(Revision 4524)
+++ sink.c	(Arbeitskopie)
@@ -121,11 +121,9 @@
 
 	    ctl->smtphost = idp->id;  /* remember last host tried. */
 	    if(ctl->smtphost[0]=='/')
-		ctl->listener = LMTP_MODE;
-
-	    if (ctl->smtphost[0]=='/')
 	    {
-		parsed_host = NULL;
+		ctl->listener = LMTP_MODE;
+		xfree(parsed_host);
 		if ((ctl->smtp_socket = UnixOpen(ctl->smtphost))==-1)
 		    continue;
 	    }
@@ -195,7 +193,6 @@
 	}
 	set_timeout(0);
 	phase = oldphase;
-    }
 
     /*
      * RFC 1123 requires that the domain name part of the
@@ -214,11 +211,13 @@
        localhost as a domain part. */
     else
 	ctl->destaddr = xstrdup("localhost");
+	xfree(parsed_host);
+    }
+    /* end if (ctl->smtp_socket == -1) */
 
     if (outlevel >= O_DEBUG && ctl->smtp_socket != -1)
 	report(stdout, GT_("forwarding to %s\n"), ctl->smtphost);
 
-    xfree(parsed_host);
     return(ctl->smtp_socket);
 }
 

-- 
Matthias Andree

[fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

[Sunil S. <sh...@bo...>]

Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
> Matthias Andree <mat...@gm...> writes:
> > Joachim Feise reported that his smtphost setting is lost/ignored since
> > 6.3.0. I could reproduce this.
> > <http://developer.berlios.de/bugs/?func=detailbug&bug_id=5849&group_id=1824>
> >
> > However, I use neither LMTP nor multidrop, so I need help determining if
> > the attached patch that I made is safe.
> 
> Hm. This is the same sink.c patch but without space changes and should
> thus be easier to review. Please comment:

Oops, my mistake. Yes, this patch is certainly required. This will
also avoid an xfree/xstrdup on every mail. I would even recommend
moving the declaration of parsed_host into the if block.

[OFFTOPIC] While debugging this, I found a weird bug relating to this
SMTP/LMTP mode. It seems that once fetchmail switches to LMTP mode,
there is no going back to SMTP mode. Try this simple fetchmail
command:

$ fetchmail -v --smtphost /this/socket/is/down,localhost

fetchmail sends LHLO to the localhost SMTP server. Note that SMTP
cannot be forced as the user could be running this configuration:

$ fetchmail -v --lmtp --smtphost localhost/port,/this/socket/is/up

where localhost is indeed meant to be an LMTP server.

The situation gets worse if fetchmail sends a bounce message in
between!

-- 
Sunil Shetye.

Re: [fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

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

Sunil Shetye <sh...@bo...> writes:

> Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
>> Matthias Andree <mat...@gm...> writes:
>> > Joachim Feise reported that his smtphost setting is lost/ignored since
>> > 6.3.0. I could reproduce this.
>> > <http://developer.berlios.de/bugs/?func=detailbug&bug_id=5849&group_id=1824>
>> >
>> > However, I use neither LMTP nor multidrop, so I need help determining if
>> > the attached patch that I made is safe.
>> 
>> Hm. This is the same sink.c patch but without space changes and should
>> thus be easier to review. Please comment:
>
> Oops, my mistake.

Is it really?

> Yes, this patch is certainly required. This will also avoid an
> xfree/xstrdup on every mail. I would even recommend moving the
> declaration of parsed_host into the if block.
>
> [OFFTOPIC] While debugging this, I found a weird bug relating to this
> SMTP/LMTP mode. It seems that once fetchmail switches to LMTP mode,
> there is no going back to SMTP mode. Try this simple fetchmail
> command:
>
> $ fetchmail -v --smtphost /this/socket/is/down,localhost
>
> fetchmail sends LHLO to the localhost SMTP server. Note that SMTP
> cannot be forced as the user could be running this configuration:

Well, one thing that astonished me a bit is that on one hand the input
side is clearly encapsulated with object-oriented practices, while the
output side (SMTP/LMTP; MDA; BSMTP) is not nearly as clearly separated.
On the other hand, we use POP3 sibling "protocols" where a distinction
per "authentication" would be sufficient.

We certainly have some tasks ahead before fetchmail 7.0.0 some day - and
I wonder if we should talk to the compilers in C++ rather than C.

> $ fetchmail -v --lmtp --smtphost localhost/port,/this/socket/is/up
>
> where localhost is indeed meant to be an LMTP server.
>
> The situation gets worse if fetchmail sends a bounce message in
> between!

Sending bounce messages is a sore spot. AFAIR, no bounce messages are
ever sent if bogofilter is in MDA mode. MDA mode doesn't work for
multidrop, as you'll probably recall from past discussions over at
fetchmail-friends. I don't know about BSMTP restrictions.

-- 
Matthias Andree

[fetchmail-devel] [PATCH] fix listener mode [WAS: Re: RFC: Patch to fix Berlios Bug #5849]

[Sunil S. <sh...@bo...>]

Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
> > [OFFTOPIC] While debugging this, I found a weird bug relating to this
> > SMTP/LMTP mode. It seems that once fetchmail switches to LMTP mode,
> > there is no going back to SMTP mode. Try this simple fetchmail
> > command:
> >
> > $ fetchmail -v --smtphost /this/socket/is/down,localhost
> >
> > fetchmail sends LHLO to the localhost SMTP server. Note that SMTP
> > cannot be forced as the user could be running this configuration:

Here is a patch which fixes this issue. This basically removes the
global smtp_mode variable and passes the desired mode in every smtp
function.

You may compare the output of

$ fetchmail -v --smtphost /this/socket/is/down,localhost

before and after the patch.

-- 
Sunil Shetye.

Re: [fetchmail-devel] [PATCH] fix listener mode

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

Sunil Shetye <sh...@bo...> writes:

> Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
>> > [OFFTOPIC] While debugging this, I found a weird bug relating to this
>> > SMTP/LMTP mode. It seems that once fetchmail switches to LMTP mode,
>> > there is no going back to SMTP mode. Try this simple fetchmail
>> > command:
>> >
>> > $ fetchmail -v --smtphost /this/socket/is/down,localhost
>> >
>> > fetchmail sends LHLO to the localhost SMTP server. Note that SMTP
>> > cannot be forced as the user could be running this configuration:
>
> Here is a patch which fixes this issue. This basically removes the
> global smtp_mode variable and passes the desired mode in every smtp
> function.

Wow, that patch is huge.  I'll happily merge it or something similar
onto the trunk (cleanup is always good), but for 6.3.1 I'd prefer a
smaller patch, even if it means hacking the state structures in some
places (for instance, upon sending a bounce and completion of the bounce
function).

I wonder if bouncing messages is the right thing at all. There should be
no reason to ever bounce in singledrop mode, and multidrop mode should
also only bounce for unknown users (but actually, that's the task of the
upstream servers; catchall OTOH is not a good idea any more nowadays).

> You may compare the output of
>
> $ fetchmail -v --smtphost /this/socket/is/down,localhost
>
> before and after the patch.

-- 
Matthias Andree

Re: [fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

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

Sunil Shetye <sh...@bo...> writes:

> $ fetchmail -v --lmtp --smtphost localhost/port,/this/socket/is/up
>
> where localhost is indeed meant to be an LMTP server.
>
> The situation gets worse if fetchmail sends a bounce message in
> between!

One more thing: the whole LMTP implementation is a mess. Why would the
kind of socket (UNIX vs. INET) determine if SMTP or LMTP is spoken by
the service?

-- 
Matthias Andree

[fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

[Sunil S. <sh...@bo...>]

Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
> Sunil Shetye <sh...@bo...> writes:
> 
> > $ fetchmail -v --lmtp --smtphost localhost/port,/this/socket/is/up
> >
> > where localhost is indeed meant to be an LMTP server.
> >
> > The situation gets worse if fetchmail sends a bounce message in
> > between!
> 
> One more thing: the whole LMTP implementation is a mess. Why would the
> kind of socket (UNIX vs. INET) determine if SMTP or LMTP is spoken by
> the service?

It looks like it is assumed that there are no SMTP UNIX sockets.

I think, it is even more complex to give the port and type with every
inet host and type with every unix socket. And there is no way of
specifying different smtp options (esmtpname, esmtppassword) per
smtphost.

This should be a TODO.

Any ideas, comments on this?

-- 
Sunil Shetye.

Re: [fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

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

Sunil Shetye <sh...@bo...> writes:

> Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
>> Sunil Shetye <sh...@bo...> writes:
>> 
>> > $ fetchmail -v --lmtp --smtphost localhost/port,/this/socket/is/up
>> >
>> > where localhost is indeed meant to be an LMTP server.
>> >
>> > The situation gets worse if fetchmail sends a bounce message in
>> > between!
>> 
>> One more thing: the whole LMTP implementation is a mess. Why would the
>> kind of socket (UNIX vs. INET) determine if SMTP or LMTP is spoken by
>> the service?
>
> It looks like it is assumed that there are no SMTP UNIX sockets.

Right, but my personal feeling is that a separate global --lmtp option
is plain ugly and undesirable, and LMTP hosts do not belong in
"--smtphost" for consistency reasons. 

I'd rather call this --mailsink (or perhaps, if someone has a better
name) and use tuples of {PROTOCOL}host/port, {PROTOCOL}[host]/port or
similar (delimiters around host similar to Postfix which goes out of the
IPv6 colon confusion when using :port).

PROTOCOL might default to SMTP if not given.

> I think, it is even more complex to give the port and type with every
> inet host and type with every unix socket. And there is no way of
> specifying different smtp options (esmtpname, esmtppassword) per
> smtphost.
>
> This should be a TODO.
>
> Any ideas, comments on this?

Yup, 6.4.0 stuff. This falls into a similar category as a hierarchical
rcfile parser with option inheritance. The current idea would be to
scope settings similar to C auto variables, and the result will then
resemble the ISC DHCP and ISC BIND configuration files.

I've added something to the TODO on the trunk.

-- 
Matthias Andree

[fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

[Sunil S. <sh...@bo...>]

Quoting from Matthias Andree's mail on Tue, Dec 13, 2005:
> Well, one thing that astonished me a bit is that on one hand the input
> side is clearly encapsulated with object-oriented practices, while the
> output side (SMTP/LMTP; MDA; BSMTP) is not nearly as clearly separated.
> On the other hand, we use POP3 sibling "protocols" where a distinction
> per "authentication" would be sufficient.

...

> Sending bounce messages is a sore spot. AFAIR, no bounce messages are
> ever sent if bogofilter is in MDA mode. MDA mode doesn't work for
> multidrop, as you'll probably recall from past discussions over at
> fetchmail-friends. I don't know about BSMTP restrictions.

I had worked on splitting up sink.c. I gave up when there were too
many local changes and not even read access to the repository then.
Also, by the time I had almost finished, a new version of fetchmail
had come out which had too many changes in sink.c.

One thing that had stumped me then was the FALLBACK_MDA feature. After
splitting, implementing FALLBACK_MDA would be difficult (but not
impossible). Should this feature be continued to be supported?

Once splitting is completed, making MDA work in multidrop should not
be a problem.

Here would be the aim of this development:

- Split sink.c into sink-smtp.c, sink-bsmtp.c, sink-mda.c (and
  possibly, sink-lmtp.c).
- Make a structure (similar to struct method) to handle the sending of
  messages.
- Make the mda code handle the exit code in accordance with
  <sysexits.h>.
- Make the mda code work in multidrop mode.
- Make the parser accept options per sink.
- Make all sink options (like esmtpname) per sink.
- Make FALLBACK_MDA work.

Potential issues involved:
- Cross platform:
  * Does <sysexits.h> exists on all platforms?
  * Do all platforms have at least EX_OK, EX__BASE, EX__MAX?
  * Do all platforms have the common exit codes that need to be
    handled specially (like EX_NOUSER)?
- How to specify options per smtphost?
- Any other issues?

Any comments on this?

-- 
Sunil Shetye.

Re: [fetchmail-devel] Re: RFC: Patch to fix Berlios Bug #5849

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

Sunil Shetye <sh...@bo...> writes:

> One thing that had stumped me then was the FALLBACK_MDA feature. After
> splitting, implementing FALLBACK_MDA would be difficult (but not
> impossible). Should this feature be continued to be supported?

I don't like this feature, and ESR agreed to change the default to be
"none" when I urged in that direction, so we've been without default
fallback for a while now.

Such fallback layers are rarely tested thoroughly and will often behave
differently enough from the "first attempt" code that I'd call such
fallback layers "nondeterministic" (for practical purposes), and I'd
rather suggest that people make sure their primary delivery target, be
it SMTP, LMTP, BSMTP or MDA, is reliable.  The last thing I want is that
people search for their mail because some fallback layer delivered it to
another place that the user has long forgotten about.

In short: I have no problem with removing the fallback code altogether.

> Once splitting is completed, making MDA work in multidrop should not
> be a problem.
>
> Here would be the aim of this development:
>
> - Split sink.c into sink-smtp.c, sink-bsmtp.c, sink-mda.c (and
>   possibly, sink-lmtp.c).

I'm not sure if sink-smtp.c, sink-bsmtp.c and sink-lmtp.c are
sufficiently different to warrant three separate modules. They share
quite a lot of code with some minor differences, but in the end it's
always an SMTP dialect. If the differences between these can be factored
out with common code shared, that'll be good. For instance, BSMTP and
SMTP are quite similar, we'll just replace a backend writer function (or
file/socket descriptor) and pretend all incoming response codes had been
the proper OK code for the respective stage (20X, 250, 354). LMTP is
also quite similar to ESMTP so that shared code is probably more
maintainable in the long run.

If C++ is of advantage (class inheritance), I don't mind either, we'll
slap "7.0.0" (rather than 6.4.0) on the new code then.

> - Make a structure (similar to struct method) to handle the sending of
>   messages.
> - Make the mda code handle the exit code in accordance with
>   <sysexits.h>.
> - Make the mda code work in multidrop mode.
> - Make the parser accept options per sink.
> - Make all sink options (like esmtpname) per sink.

> - Make FALLBACK_MDA work.
>
> Potential issues involved:
> - Cross platform:
>   * Does <sysexits.h> exists on all platforms?

Ah well... it's mail system specific, originated AFAICS in BSD4.3 and
was also present in System V.  It should therefore be fairly portable,
but we might want to peek at Postfix's source code to see if sysexits.h
specific workarounds are there.

>   * Do all platforms have at least EX_OK, EX__BASE, EX__MAX?
>   * Do all platforms have the common exit codes that need to be
>     handled specially (like EX_NOUSER)?
> - How to specify options per smtphost?

Well, a configuration system with inheritance was already suggested, but
would be a somewhat larger change, too.

> - Any other issues?

How to bounce (or what is other good non-bounce behavior) in MDA mode?

> Any comments on this?

-- 
Matthias Andree

[fetchmail-devel] Re: fix listener mode

[Sunil S. <sh...@bo...>]

Quoting from Matthias Andree's mail on Mon, Dec 19, 2005:
> > Here is a patch which fixes this issue. This basically removes the
> > global smtp_mode variable and passes the desired mode in every smtp
> > function.
> 
> Wow, that patch is huge.  I'll happily merge it or something similar
> onto the trunk (cleanup is always good), but for 6.3.1 I'd prefer a
> smaller patch, even if it means hacking the state structures in some
> places (for instance, upon sending a bounce and completion of the bounce
> function).

There are two problems that that patch fixes:

- After switching to LMTP mode when trying out multiple smtphosts,
  fetchmail never switches back to SMTP mode and continues talking
  LMTP to the next SMTP socket. In daemon mode, it will not even talk
  SMTP on the next poll of that mailserver.

$ fetchmail -v --smtphost /this/socket/is/down,localhost

  The source of this problem is that ctl->listener is never reset to
  SMTP_MODE.

- After sending a bounce, fetchmail does not switch back to LMTP mode
  and continues talking SMTP to the LMTP socket. In daemon mode,
  however, it will talk LMTP on the next run.

  The source of this problem is that the global variable smtp_mode is
  never reset to LMTP_MODE in that poll.

A smaller patch can fix only one of the two issues. I assume that you
are interested in keeping the global variable smtp_mode. The problem
is that fetchmail has too many return points (including return via
longjump()) and ensuring that smtp_mode is indeed reset to its
original value in all cases is not going to really make the patch
smaller.

If you wish, you may instead apply the patch against the trunk only.

-- 
Sunil Shetye.

Re: [fetchmail-devel] Re: fix listener mode

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

Sunil Shetye <sh...@bo...> writes:

>> Wow, that patch is huge.  I'll happily merge it or something similar
>> onto the trunk (cleanup is always good), but for 6.3.1 I'd prefer a
>> smaller patch, even if it means hacking the state structures in some
>> places (for instance, upon sending a bounce and completion of the bounce
>> function).
>
> There are two problems that that patch fixes:
>
> - After switching to LMTP mode when trying out multiple smtphosts,
>   fetchmail never switches back to SMTP mode and continues talking
>   LMTP to the next SMTP socket. In daemon mode, it will not even talk
>   SMTP on the next poll of that mailserver.
>
> $ fetchmail -v --smtphost /this/socket/is/down,localhost
>
>   The source of this problem is that ctl->listener is never reset to
>   SMTP_MODE.
>
> - After sending a bounce, fetchmail does not switch back to LMTP mode
>   and continues talking SMTP to the LMTP socket. In daemon mode,
>   however, it will talk LMTP on the next run.
>
>   The source of this problem is that the global variable smtp_mode is
>   never reset to LMTP_MODE in that poll.
>
> A smaller patch can fix only one of the two issues. I assume that you
> are interested in keeping the global variable smtp_mode. The problem
> is that fetchmail has too many return points (including return via
> longjump()) and ensuring that smtp_mode is indeed reset to its
> original value in all cases is not going to really make the patch
> smaller.

OK. I'll consider the full thing for 6.3.2, but as the bug has been
found by you (as developer) and not by a user, I am inclined to assume
that LMTP is not in wide use (and after all, many sites interested in
LMTP would be using Cyrus, which has a "deliver" program, too), or if it
is, there are no SMTP fallback layers and perhaps no bounces.

I don't see why a site would use mixed LMTP/SMTP delivery. It's the same
story as with fallback MDA: it just introduces inconsistencies by going
several inbound paths, which many sites will probably rather avoid.

-- 
Matthias Andree

Re: [fetchmail-devel] Re: fix listener mode

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

Matthias Andree <mat...@gm...> writes:

> OK. I'll consider the full thing for 6.3.2, but as the bug has been
> found by you (as developer) and not by a user, I am inclined to assume
> that LMTP is not in wide use (and after all, many sites interested in
> LMTP would be using Cyrus, which has a "deliver" program, too), or if it
> is, there are no SMTP fallback layers and perhaps no bounces.

I've merged your patch for the trunk and on the 6.3 branch.

Thanks a lot for your work. Your argumentation of not being able to
track the SMTP/LMTP mode properly convinced me to take this patch in
spite of its size.

-- 
Matthias Andree