qpsmtpd-async: premature end of data problem
Radu Greab
I found a case where qpsmtpd-async detects the end of data marker
incorrectly: the previous packet did not end with CRLF and the current
packet starts with .CRLF. The code assumes that the previous packet
ended with CRLF.
Attached are a test script and a suggested patch.
Thanks,
Radu Greab
a...@develooper.com
On Apr 12, 2008, at 6:33, Radu Greab wrote:
> I found a case where qpsmtpd-async detects the end of data marker
> incorrectly: the previous packet did not end with CRLF and the current
> packet starts with .CRLF. The code assumes that the previous packet
> ended with CRLF.
Hi Radu,
Can you add it to the issue tracker?
http://code.google.com/p/smtpd/issues/list
Wouldn't the non-pollserver servers have the same problem?
- ask
--
http://develooper.com/ - http://askask.com/
Matt Sergeant
>
> On Apr 12, 2008, at 6:33, Radu Greab wrote:
>
>> I found a case where qpsmtpd-async detects the end of data marker
>> incorrectly: the previous packet did not end with CRLF and the current
>> packet starts with .CRLF. The code assumes that the previous packet
>> ended with CRLF.
>
> Hi Radu,
>
> Can you add it to the issue tracker?
>
> http://code.google.com/p/smtpd/issues/list
>
> Wouldn't the non-pollserver servers have the same problem?
No, because they do <$fh> and block until there's a full line.
Though I could have sworn I fixed this - are you sure you're testing
against latest SVN?
Matt.
Radu Greab
Matt Sergeant wrote:
>
> Though I could have sworn I fixed this - are you sure you're testing
> against latest SVN?
Hi,
I do not have the latest SVN in production. I have 0.43rc1 with
additional patches, including your change #129 which, I think, is what
you are referring to as having fixed the issue. However the patch was
developed and tested against the latest SVN and the latest SVN has the
problem too.
In this new case the header was already read. Qpsmtpd received a packet
starting with .CRLF and it thought it was the end-of-data marker. In
fact, it was just the end of the last sentence from the previous packet.
Should I try to improve the test script? It works for me, I can
reproduce the problem with it.
Thanks,
Radu Greab
Radu Greab
Radu Greab wrote:
>
>
> Matt Sergeant wrote:
> >
> > Though I could have sworn I fixed this - are you sure you're testing
> > against latest SVN?
>
> additional patches, including your change #129 which, I think, is what
> you are referring to as having fixed the issue.
Sorry, 129 is the revision number from my local svk mirror. I should
have said revision #863.
Matt Sergeant
So the test script is doing the wrong thing. Any "." at the start of a
line in SMTP needs to be "encoded" as ".." and the SMTP line processor
decodes it to a single dot.
So the test is wrong, and Qpsmtpd is doing the right thing in assuming the
end of data has been reached. At least as far as I can tell.
Matt.
Matt Sergeant
Ugh. Ignore me. Need more coffee :-)
Matt Sergeant
I committed the patch 95% the same as yours.
I can't help feeling there's a better way to do it - I hate applying two
regexps every time a DATA packet comes in... I'll have a think on it.
Matt.
Guy Hulbert
> > Attached are a test script and a suggested patch.
>
> I committed the patch 95% the same as yours.
>
> I can't help feeling there's a better way to do it - I hate applying
> two
> regexps every time a DATA packet comes in... I'll have a think on it.
tr ?
--
--gh
Matt Sergeant
I'm thinking more on the lines of pushing back reads if they're not full
lines.
Probably needs a bit of support for that in lib/Danga/Client.pm
Matt.
Radu Greab
Matt Sergeant wrote:
>
> >> I can't help feeling there's a better way to do it - I hate applying
> >> two
> >> regexps every time a DATA packet comes in... I'll have a think on
> >> it.
One option could be to keep this regexp
$data =~ s/\r\n\.\r\n(.*)\z/\r\n/ms
and, if it is not matched, then check if the packet ends with CRLF. If
true, then push back to Danga::Client those CRLF, for the next
invocation of the callback. Please check the attached patch for what I'm
thinking at. It needs the Danga::Client change and testing.
> I'm thinking more on the lines of pushing back reads if they're not full
> lines.
Just thinking aloud: could that be used by malevolent people? Like they
send valid data lines, but they take care to never end a packet with
CRLF. If this looks plausible, then Danga::Client needs to protect
against that attack?
Thanks,
Radu Greab
Juerd Waalboer
> $data =~ s/\r\n\.\r\n(.*)\z/\r\n/ms
If you don't want to use the regex engine (and indeed, especially
regexes with .* are often inefficient), you could use good old index and
substr to achieve the same:
# untested code
my $data_end = index($data, "\r\n.\r\n");
if ($data_end >= 0) {
substr($data, $data_end + 2, length($data) - ($data_end + 2), "");
...
}
I'm not familiar with the rest of the code and I have not attempted to
understand its workings. This suggestion may not work in the given
context. Use with caution, etc, bla bla.
Also, I have not benchmarked this.
--
Met vriendelijke groet, Kind regards, Korajn salutojn,
Juerd Waalboer: Perl hacker <#####@juerd.nl> <http://juerd.nl/sig>
Convolution: ICT solutions and consultancy <sa...@convolution.nl>
Radu Greab
Matt Sergeant wrote:
>
> I'm thinking more on the lines of pushing back reads if they're not full
> lines.
>
> Probably needs a bit of support for that in lib/Danga/Client.pm
Attached is an attempt to do that. The reason for this is that I've seen
another case of end of data marker coming in two different packets and
the previous fix does not work: one packet ends with CRLF. , the
following packet starts with CRLF. A test script is attached too.
This new attempt reverts all previous changes to Qpsmtpd::PollServer and
changes Danga::Client to send to the callback full lines: either the
entire packet if it ends with CRLF or the part up to the last CRLF,
inclusive. What is left, a possible partial line, is buffered and
processed when the next packet comes in.
Thanks,
Radu Greab