Bug #84941 for Sendmail-PMilter: Can't fetch correct macros, can't set some headers

Tue Apr 30 10:40:35 2013 perl [...] cmadams.net - Ticket created

Subject:	Can't fetch correct macros, can't set some headers
Date:	Tue, 30 Apr 2013 09:40:16 -0500
To:	bug-Sendmail-PMilter [...] rt.cpan.org
From:	Chris Adams <perl [...] cmadams.net>

I have found a couple of bugs in version 1.00 of Sendmail::PMilter (in the Sendmail::PMilter::Context module): - The main loop stores macros by milter state, but $ctx->getsymval looks in a pre-set subset of the states. This means that macros set in other states (such as EOM) can't be fetched. A quick-and-dirty change is to look in all known states: - foreach my $code (SMFIC_RCPT, SMFIC_MAIL, SMFIC_HELO, SMFIC_CONNECT) { + foreach my $code (SMFIC_UNKNOWN, SMFIC_DATA, SMFIC_QUIT, SMFIC_RCPT, + SMFIC_OPTNEG, SMFIC_EOH, SMFIC_MAIL, SMFIC_HEADER, SMFIC_HELO, + SMFIC_BODYEOB, SMFIC_MACRO, SMFIC_CONNECT, SMFIC_BODY, + SMFIC_ABORT) { A better fix may actually be to just store macros in a single hash as they come in. I don't think there is any reason to keep them separate. - $ctx->addheader checks for a header name/value setting with "||", which fails when trying to set a header value of "0" (for example, a spam score). I fixed this with: - my $header = shift || die "addheader: no header name\n"; - my $value = shift || die "addheader: no header value\n"; + my $header = shift; + my $value = shift; + die "addheader: no header name\n" if (! defined ($header)); + die "addheader: no header value\n" if (! defined ($value)); -- Chris Adams <perl@cmadams.net>

Tue Apr 30 11:58:08 2013 ZUMMO [...] cpan.org - Correspondence added

Show quoted text

> A better fix may actually be to just store macros in a single hash as > they come in. I don't think there is any reason to keep them > separate. >

The code is pretty complicated, so there might be a reason for this. Maybe you'd want to check the milter specs to see if there's any reference to that. Show quoted text

> - $ctx->addheader checks for a header name/value setting with "||", which > fails when trying to set a header value of "0" (for example, a spam > score). I fixed this with:

thanks. I've committed a fix to the github repo for this module.

Tue Apr 30 11:58:09 2013 The RT System itself - Status changed from 'new' to 'open'

Tue Apr 30 11:58:09 2013 ZUMMO [...] cpan.org - Taken

Thu May 02 10:48:55 2013 perl [...] cmadams.net - Correspondence added

Subject:	Re: [rt.cpan.org #84941] Can't fetch correct macros, can't set some headers
Date:	Thu, 2 May 2013 09:48:31 -0500
To:	Alessandro Zummo via RT <bug-Sendmail-PMilter [...] rt.cpan.org>
From:	Chris Adams <perl [...] cmadams.net>

Once upon a time, Alessandro Zummo via RT <bug-Sendmail-PMilter@rt.cpan.org> said: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=84941 >

> > A better fix may actually be to just store macros in a single hash as > > they come in. I don't think there is any reason to keep them > > separate.

> > The code is pretty complicated, so there might be a reason for this. Maybe > you'd want to check the milter specs to see if there's any reference to that.

The milter API says: All macros stay in effect from the point they are received until the end of the connection for the first two sets, the end of the message for the third (xxfi_envfrom) and last (xxfi_eom), and just for each recipient for xxfi_envrcpt. So you are right, they need to be kept separate to keep the correct scope. My quick-and-dirty patch added ALL states, which is overkill (since many of them _can't_ send macros). The above doesn't specify the scope for macros received with xxfi_data and xxfi_eoh (I would assume both are until xxfi_eom though). Here's a revised (but untested yet) patch that sets that scope and macro list. How does this look to you? diff -urN Sendmail-PMilter-1.00-dist/lib/Sendmail/PMilter/Context.pm Sendmail-PMilter-1.00/lib/Sendmail/PMilter/Context.pm --- Sendmail-PMilter-1.00-dist/lib/Sendmail/PMilter/Context.pm 2011-04-16 08:07:43.000000000 -0500 +++ Sendmail-PMilter-1.00/lib/Sendmail/PMilter/Context.pm 2013-05-02 09:46:28.000000000 -0500 @@ -247,6 +247,10 @@ } } elsif ($cmd eq SMFIC_BODYEOB) { $this->call_hooks('eom'); + delete $this->{symbols}{&SMFIC_MAIL}; + delete $this->{symbols}{&SMFIC_DATA}; + delete $this->{symbols}{&SMFIC_EOH}; + delete $this->{symbols}{&SMFIC_BODYEOB}; } elsif ($cmd eq SMFIC_HELO) { my $helo = &$split_buf; die "SMFIC_HELO: bad packet\n" unless (@$helo == 1); @@ -472,7 +476,7 @@ my $this = shift; my $key = shift; - foreach my $code (SMFIC_RCPT, SMFIC_MAIL, SMFIC_HELO, SMFIC_CONNECT) { + foreach my $code (SMFIC_CONNECT, SMFIC_HELO, SMFIC_MAIL, SMFIC_RCPT, SMFIC_DATA, SMFIC_EOH, SMFIC_BODYEOB) { my $val = $this->{symbols}{$code}{$key}; return $val if defined($val); -- Chris Adams <perl@cmadams.net>

Sun Apr 08 08:37:21 2018 pause [...] jubileegroup.co.uk - Correspondence added

Hi, I'd like to take ownership of this and all other Sendmail::PMilter tickets. I have a current version with most of the fixes applied, updating the docs, thinking about version numbering etc and checking for compatibility with old versions at the moment with a view to releasing a fixed version later this year. OK with you?

Sun Jun 30 13:02:31 2019 pause [...] jubileegroup.co.uk - Correspondence added

Fixed in version 1.21, not yet released.

Sun Jun 30 13:02:31 2019 pause [...] jubileegroup.co.uk - Status changed from 'open' to 'resolved'

Sat Jul 13 05:19:58 2019 pause [...] jubileegroup.co.uk - Fixed in 1.20_01 added