Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: exim: dev

Re: Refactor optional MAIL args processing

 

 

exim dev RSS feed   Index | Next | Previous | View Threaded


tlyons at ivenue

Mar 29, 2012, 12:11 PM

Post #1 of 7 (268 views)
Permalink
Re: Refactor optional MAIL args processing

On Fri, Mar 23, 2012 at 8:59 AM, Todd Lyons <tlyons [at] ivenue> wrote:
>>> Quoting Jeremy Harris:
>>> I'd like a code refactor from the if/elseif chain into a table-driven
>>> approach, using a static data table of acceptable option names
>
> I used the method and techniques your patch implemented to refactor
> the if else chain for MAIL smtp command option processing inside the
> for() loop into a switch() construction.  It passes all my manual
> tests (interesting that swaks doesn't have any option that passes
> optional args to the MAIL FROM) on my test system. I'll roll it live
> next week and watch for issues (waiting until after the weekend).  If
> it works ok for a week and I can verify that it's actually being
> exercised, then I'll submit it to the exim team.

So far it's working well. The attached patch is based on the
exim-4_77 tag, but it applies to master cleanly. It has a couple of
trailing whitespaces, just add '--whitespace=fix' to the git am
command. Can anybody look at the attached patch and tell me:

1) Is it conforming to exim standards?
2) Does it result in sane looking code?
3) Are there any glaring errors?

Comments and feedback are welcome.

...Todd
--
Always code as if the guy who ends up maintaining your code will be a
violent psychopath who knows where you live. -- Martin Golding
Attachments: 0001-Refactor-MAIL-FROM-optional-args-processing.patch (10.8 KB)


pdp at exim

Mar 29, 2012, 5:26 PM

Post #2 of 7 (252 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

On 2012-03-29 at 12:11 -0700, Todd Lyons wrote:
> 1) Is it conforming to exim standards?

We have standards? Why does nobody tell me these things?

More seriously: there are several different code styles in the project;
just try to persist the style of the file you're touching.

> 2) Does it result in sane looking code?

I'm almost asleep and haven't applied the patch, but what I get mentally
from reading the diff looks somewhat sane.

> Comments and feedback are welcome.

I'm glad to see this happening. :)

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


tlyons at ivenue

Apr 12, 2012, 1:43 PM

Post #3 of 7 (215 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

Sorry for the HTML email, but I wanted to retain long lines.

On Thu, Mar 29, 2012 at 5:26 PM, Phil Pennock <pdp [at] exim> wrote:
>> 2) Does it result in sane looking code?
> I'm almost asleep and haven't applied the patch, but what I get mentally
> from reading the diff looks somewhat sane.

Ok, it ran for almost a week, and then someone reported an error that
looked very much like my code was to blame, so I backed it out. Today I
tracked it down, and it was not the changes I made, instead the error is
due to exim's strict adherence to RFC 5321. The reported error was similar
to:

501 <todd [at] mrball> BODY=8BITMIME: malformed address: BODY=8BITMIME may
not follow <todd [at] mrball>

You can't see it but there is a trailing tab in the string
"<todd [at] mrball>\t"
(both of them). Exim appears to conform exactly to the RFC which says that
a space must separate the email address from optional arguments. Quoting
RFC 5321:

In some commands and replies, arguments are required following the verb or
reply code. Some commands do not accept arguments (after the verb), and
some reply codes are followed, sometimes optionally, by free form text. In
both cases, where text appears, it is separated from the verb or reply code
by a space character.
<snip>
MAIL FROM:<reverse-path> [SP <mail-parameters> ] <CRLF>

The function extract_option() in smtp_in.c parses the MAIL FROM commandline
for optional arguments, stores them in various variables, then finally
returns just the email address, which is then extracted using
parse_extract_address(). It accepts any whitespace (!isspace()) as it
traverses the string looking for optional arguments, but it accepts only a
space character as the separator between them in a later check. The
following is valid:

MAIL FROM:<todd [at] mrball>[SPACE]BODY=8BITMIME

And this is invalid according to exim:

MAIL FROM:<todd [at] mrball>[TAB]BODY=8BITMIME

I did some testing and every other MTA I can find accepts a TAB as a valid
separator, exim seems to be the only one who doesn't allow it. A quick fix
doesn't appear to be too intrusive, but there may be better ways of solving
it. This is in the extract_option() function. The line numbers will be
off since I have some other modifications in this file:

diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c
index 6d8d5c7..0e7d8bf 100644
--- a/src/src/smtp_in.c
+++ b/src/src/smtp_in.c
@@ -1001,7 +1001,8 @@ if (*v != '=') return FALSE;
n = v;
while(isalpha(n[-1])) n--;

-if (n[-1] != ' ') return FALSE;
+if (n[-1] == '\t') n[-1] = ' ';
+else if (n[-1] != ' ') return FALSE;

n[-1] = 0;
*name = n;


Is this something that would be considered? This is relaxing adherence to
an RFC to make it more comparable to other MTA's, all others that I tested
(gmail, qmail, sendmail, postfix). IMHO this is so minor that making a
global setting in exim to control it is a waste. But relaxing RFC
compliance is no light matter either.

I have not tested this against the test suite, I still have not
successfully gotten it to run yet.

...Todd
--
Always code as if the guy who ends up maintaining your code will be a
violent psychopath who knows where you live. -- Martin Golding
--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


pdp at exim

Apr 12, 2012, 7:23 PM

Post #4 of 7 (215 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

On 2012-04-12 at 13:43 -0700, Todd Lyons wrote:
> I did some testing and every other MTA I can find accepts a TAB as a valid
> separator, exim seems to be the only one who doesn't allow it. A quick fix
> doesn't appear to be too intrusive, but there may be better ways of solving
> it. This is in the extract_option() function. The line numbers will be
> off since I have some other modifications in this file:

Your fix looks good to me.

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


pdp at exim

Apr 12, 2012, 7:38 PM

Post #5 of 7 (215 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

On 2012-04-12 at 19:23 -0700, Phil Pennock wrote:
> On 2012-04-12 at 13:43 -0700, Todd Lyons wrote:
> > I did some testing and every other MTA I can find accepts a TAB as a valid
> > separator, exim seems to be the only one who doesn't allow it. A quick fix
> > doesn't appear to be too intrusive, but there may be better ways of solving
> > it. This is in the extract_option() function. The line numbers will be
> > off since I have some other modifications in this file:
>
> Your fix looks good to me.

Went for a minor variation, using isspace(); the only bad character
matched by that is \n which can't be present anyway. Seemed better than
setting it to space, before an else check which wouldn't be applied
anyway, only to set it to \0 a moment later. Two tests is more work
than a 256 byte table lookup.

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


tlyons at ivenue

Apr 13, 2012, 7:44 AM

Post #6 of 7 (210 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

On Thu, Apr 12, 2012 at 7:38 PM, Phil Pennock <pdp [at] exim> wrote:
>> > I did some testing and every other MTA I can find accepts a TAB as a valid
>> > separator, exim seems to be the only one who doesn't allow it.  A quick fix
>> > doesn't appear to be too intrusive, but there may be better ways of solving
>> > it.  This is in the extract_option() function.  The line numbers will be
>> > off since I have some other modifications in this file:
> Went for a minor variation, using isspace(); the only bad character
> matched by that is \n which can't be present anyway.  Seemed better than
> setting it to space, before an else check which wouldn't be applied
> anyway, only to set it to \0 a moment later.  Two tests is more work
> than a 256 byte table lookup.

Understood. Passes my testing now. Thanks Phil!

...Todd
--
Always code as if the guy who ends up maintaining your code will be a
violent psychopath who knows where you live. -- Martin Golding

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


tlyons at ivenue

Apr 13, 2012, 8:04 AM

Post #7 of 7 (212 views)
Permalink
Re: Refactor optional MAIL args processing [In reply to]

On Fri, Apr 13, 2012 at 7:44 AM, Todd Lyons <tlyons [at] ivenue> wrote:
> On Thu, Apr 12, 2012 at 7:38 PM, Phil Pennock <pdp [at] exim> wrote:
>>> > I did some testing and every other MTA I can find accepts a TAB as a valid
>>> > separator, exim seems to be the only one who doesn't allow it.  A quick fix
>>> > doesn't appear to be too intrusive, but there may be better ways of solving
>>> > it.  This is in the extract_option() function.  The line numbers will be
>>> > off since I have some other modifications in this file:
>> Went for a minor variation, using isspace(); the only bad character
> Understood.  Passes my testing now.  Thanks Phil!

Interesting, an overnight packet dump revealed that it sometimes isn't
a tab. In the one below, it is a space. The cause is that more than
one optional parameter is being passed. It works properly on standard
exim 4.77, but with my refactoring patch, it does not. I'll bet I
don't reset the state variable of the options processing (reversed the
order of BODY= and SIZE= and it works ok).

T 219.87.80.49:3880 -> 208.89.138.22:25 [AP]
4d 41 49 4c 20 46 52 4f 4d 3a 3c 50 65 67 67 79 MAIL FROM:<Peggy
5f 58 69 65 40 73 64 63 2e 73 65 72 63 6f 6d 6d _Xie [at] sdc
2e 63 6f 6d 3e 20 42 4f 44 59 3d 38 42 49 54 4d .com> BODY=8BITM
49 4d 45 20 53 49 5a 45 3d 32 38 34 38 0d 0a 52 IME SIZE=2848..R
43 50 54 20 54 4f 3a 3c 72 6f 62 65 72 74 40 65 CPT TO:<robert@e
76 65 72 63 61 6d 65 6c 2e 63 6f 6d 2e 74 77 3e vercamel.com.tw>
0d 0a 52 43 50 54 20 54 4f 3a 3c 6b 69 6b 69 40 ..RCPT TO:<kiki@
65 76 65 72 63 61 6d 65 6c 2e 63 6f 6d 2e 74 77 evercamel.com.tw
3e 0d 0a 44 41 54 41 0d 0a >..DATA..


T 208.89.138.22:25 -> 219.87.80.49:3880 [AP]
35 30 31 20 3c 50 65 67 67 79 5f 58 69 65 40 73 501 <Peggy_Xie@s
64 63 2e 73 65 72 63 6f 6d 6d 2e 63 6f 6d 3e 20 dc.sercomm.com>
42 4f 44 59 3d 38 42 49 54 4d 49 4d 45 3a 20 6d BODY=8BITMIME: m
61 6c 66 6f 72 6d 65 64 20 61 64 64 72 65 73 73 alformed address
3a 20 42 4f 44 59 3d 38 42 49 54 4d 49 4d 45 20 : BODY=8BITMIME
6d 61 79 20 6e 6f 74 20 66 6f 6c 6c 6f 77 20 3c may not follow <
50 65 67 67 79 5f 58 69 65 40 73 64 63 2e 73 65 Peggy_Xie [at] sdc
72 63 6f 6d 6d 2e 63 6f 6d 3e 20 0d 0a 35 30 33 rcomm.com> ..503
20 73 65 6e 64 65 72 20 6e 6f 74 20 79 65 74 20 sender not yet
67 69 76 65 6e 0d 0a 35 30 33 20 73 65 6e 64 65 given..503 sende
72 20 6e 6f 74 20 79 65 74 20 67 69 76 65 6e 0d r not yet given.
0a 35 30 33 2d 41 6c 6c 20 52 43 50 54 20 63 6f .503-All RCPT co
6d 6d 61 6e 64 73 20 77 65 72 65 20 72 65 6a 65 mmands were reje
63 74 65 64 20 77 69 74 68 20 74 68 69 73 20 65 cted with this e
72 72 6f 72 3a 0d 0a 35 30 33 2d 35 30 33 20 73 rror:..503-503 s
65 6e 64 65 72 20 6e 6f 74 20 79 65 74 20 67 69 ender not yet gi
76 65 6e 0d 0a 35 30 33 20 56 61 6c 69 64 20 52 ven..503 Valid R
43 50 54 20 63 6f 6d 6d 61 6e 64 20 6d 75 73 74 CPT command must
20 70 72 65 63 65 64 65 20 44 41 54 41 0d 0a precede DATA..


Just wanted to point out that my previous refactoring patch still was
not working properly.

...Todd
--
Always code as if the guy who ends up maintaining your code will be a
violent psychopath who knows where you live. -- Martin Golding

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

exim dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.