Milter-greylist

Milter-greylist-4.1.10 seems to have a problem with handling
(extended) regular expressions.

From the log:
Feb  4 08:53:49 sun milter-greylist: regex
//[0-9]+([^0-9])[0-9]+\1[0-9]+\1[0-9]+// against "[122.231.167.11]":
match[1] = "."

After this, the daemon segfaults. The back trace reads:

Core was generated by `/usr/bin/milter-greylist -P
/var/milter-greylist/milter-greylist.pid -p /var/mi'.
Program terminated with signal 6, Aborted.
#0  0x00110402 in __kernel_vsyscall ()
(gdb) bt
#0  0x00110402 in __kernel_vsyscall ()
#1  0x00c34ba0 in raise () from /lib/libc.so.6
#2  0x00c364b1 in abort () from /lib/libc.so.6
#3  0x00c6adfb in __libc_message () from /lib/libc.so.6
#4  0x00c760db in free () from /lib/libc.so.6
#5  0x0804aadf in smtp_reply_free (sr=0x2) at milter-greylist.c:2083
#6  0x0804c5ce in mlfi_close (ctx=0xb610e2b3) at milter-greylist.c:1192
#7  0x080678b8 in mi_engine ()
#8  0x080645c8 in mi_handle_session ()
#9  0x08062dad in mi_thread_handle_wrapper ()
#10 0x00db645b in start_thread () from /lib/libpthread.so.0
#11 0x00cda23e in clone () from /lib/libc.so.6
(gdb)

Going through the code, it may just be that sr->sr_nmatch in
smtp_reply_free() is not reset properly after a match?

--- In milter-greylist@yahoogroups.com, "reschauzier" 
> 
> Going through the code, it may just be that sr->sr_nmatch in
> smtp_reply_free() is not reset properly after a match?
>

Yes, this indeed turned out to be the case. It is fixed by setting
sr->sr_nmatch to zero in smtp_reply_init().

I have uploaded a fix for v4.1.10:
http://files.eschauzier.org/milter-greylist/no_autowhite-4.1.10-2.patch

Besides the regex change, the file also includes a patch that rolls
all of the greylist and autowhite action into a single list,
effectively eliminating the autowhitelist. This mod is based on the
observation that a tuple entry can only exist in one of the pending
and autowhite lists at any given time.

Merging the lists into one, practically means that if a greylisted
tuple comes in for a second time, the tuple is found in the combined
list, after which the type of the entry only needs to change to
autowhite (instead of deleting the entry in the pending list, and
creating a new entry in the autowhite list).

Combining the two lists should give a good performance boost, since
milter-greylist only needs to traverse a single list, instead of two,
for each message that comes in.

Also, since all the files relating the the autowhite list have
disappeared, this is one of the rare occasions that the code actually
becomes smaller with time ;)

reschauzier <reschauzier@...> wrote:

> Yes, this indeed turned out to be the case. It is fixed by setting
> sr->sr_nmatch to zero in smtp_reply_init().

I fear you have a problem with your compiler. The zero'ing you did is
three lines below this:
        memset(sr, 0, sizeof(*sr));

So the whole structure has already been zero'ed. Do you use -O? Can you
try without any compiler optimization (and without your fix)?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

> I fear you have a problem with your compiler. The zero'ing you did is
> three lines below this:
>         memset(sr, 0, sizeof(*sr));

Yes, you are right. And in fact the problem has come back after
setting nmatch to zero. The problem is the seg fault does not seem to
be very predictable. Before setting nmatch to zero, Milter greylist
would segfault within 15 min on the first regex match. After the fix,
it has run for many hours, hitting many regexes. That's why I figured
it must have been resolved. But this was probably just coincidence,
since 1) the fix shouldn't do anything as you pointed out and 2) the
seg fault has returned.

> try without any compiler optimization (and without your fix)?

I will do that.

reschauzier wrote:
 > 
 > > I fear you have a problem with your compiler. The zero'ing you did is
 > > three lines below this:
 > >         memset(sr, 0, sizeof(*sr));
 > 

Why don't you use bzero(), BTW?

Best regards
   Oliver

-- 
Oliver Fromme, secnetix GmbH & Co. KG, Marktplatz 29, 85567 Grafing b. M.
Handelsregister: Registergericht Muenchen, HRA 74606,  Gesch\ufffdftsfuehrung:
secnetix Verwaltungsgesellsch. mbH, Handelsregister: Registergericht M\ufffdn-
chen, HRB 125758,  Gesch\ufffdftsf\ufffdhrer: Maik Bachmann, Olaf Erb, Ralf Gebhart

FreeBSD-Dienstleistungen, -Produkte und mehr:  http://www.secnetix.de/bsd

"It combines all the worst aspects of C and Lisp:  a billion different
sublanguages in one monolithic executable.  It combines the power of C
with the readability of PostScript."
        -- Jamie Zawinski, when asked: "What's wrong with perl?"

Oliver Fromme <olli@...> wrote:

> Why don't you use bzero(), BTW?

Why should I? 

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

manu@... wrote:
 > Oliver Fromme <olli@...> wrote:
 > 
 > > Why don't you use bzero(), BTW?
 > 
 > Why should I? 

Well, there's no strong technical reason, of course.
memset() is not wrong.

I was just wondering because bzero()'s purpose is
specifically to fill an area of memory with zeros.

It saves a few bytes in the binary and a few cycles
during execution, but it's not significant.  (It's
even possible that the compiler optimizes a call
to memset(..., 0, ...) with constant 0 to bzero(),
but I'm not sure if common compilers are that smart.)

Best regards
   Oliver

-- 
Oliver Fromme, secnetix GmbH & Co. KG, Marktplatz 29, 85567 Grafing b. M.
Handelsregister: Registergericht Muenchen, HRA 74606,  Gesch\ufffdftsfuehrung:
secnetix Verwaltungsgesellsch. mbH, Handelsregister: Registergericht M\ufffdn-
chen, HRB 125758,  Gesch\ufffdftsf\ufffdhrer: Maik Bachmann, Olaf Erb, Ralf Gebhart

FreeBSD-Dienstleistungen, -Produkte und mehr:  http://www.secnetix.de/bsd

"[...]  one observation we can make here is that Python makes
an excellent pseudocoding language, with the wonderful attribute
that it can actually be executed."  --  Bruce Eckel

Greg Troxel wrote:
 > 
 >   I was just wondering because bzero()'s purpose is
 >   specifically to fill an area of memory with zeros.
 > 
 >   It saves a few bytes in the binary and a few cycles
 >   during execution, but it's not significant.  (It's
 >   even possible that the compiler optimizes a call
 >   to memset(..., 0, ...) with constant 0 to bzero(),
 >   but I'm not sure if common compilers are that smart.)
 > 
 > IIRC basically bzero is a BSD thing, and memset is C99, and I think nthe
 > NetBSD codebase has moved to memset and memcp from bzero and bcopy.

bzero() is POSIX, so portability should not be a concern.

Don't get me wrong, I'm not saying the code should be
changed.  I was really just wondering.

Best regards
   Oliver

-- 
Oliver Fromme, secnetix GmbH & Co. KG, Marktplatz 29, 85567 Grafing b. M.
Handelsregister: Registergericht Muenchen, HRA 74606,  Gesch\ufffdftsfuehrung:
secnetix Verwaltungsgesellsch. mbH, Handelsregister: Registergericht M\ufffdn-
chen, HRB 125758,  Gesch\ufffdftsf\ufffdhrer: Maik Bachmann, Olaf Erb, Ralf Gebhart

FreeBSD-Dienstleistungen, -Produkte und mehr:  http://www.secnetix.de/bsd

"I have stopped reading Stephen King novels.
Now I just read C code instead."
        -- Richard A. O'Keefe

Allright, I got to the bottom of it. Turns out there is a real problem
with the way 4.1.10 (and before) handles parenthesized substring
matches in regular expressions. A clause like this:

acl greylist rcpt /(.*)/

will sooner rather than later cause a seg fault. Futhermore a clause like:

acl greylist rcpt /(unlikely_to_match)/

will cause a significant memory leak. The problem is not limited to
rcpt clauses and affects any regex in the access control list.

I have prepared a patch to resolve the seg fault in 4.1.10:

http://files.eschauzier.org/milter-greylist/regex_segfault-4.1.10.patch

It also cleans up some of the regex code. The issues resolved are the
following (in order of importance):

1. Fix segmentation fault with parenthesized substring matches in
regular expressions

Cause: the first set of pointers for storing the substring matches was
not initialized.

Remedy: move bzero statement to proper location in code

2. Memory leak with unmatched substrings

Cause: any time a regular expression with parenthesized substrings was
evaluated, but did not match, memory was allocated for storing the
substring matches, that was never freed.

Remedy: add appropriate free staments

3. Everytime a parenthesized substring match was being evaluated,
whether a match or not, a significant amount of reallocs, memmoves and
bzeroes took place, moving NULL pointer around.

Cause: memory was allocated for substring matches before an acutal
match occurred.

Remedy: allocate memory for substring matches after successful match

4. clause lists were evaluated in the opposite order of the conf file.
Eg. a acl statement like

list "test" domain {str1, str2, str3}

would be matched against str3 first, than str2, etc. Allthough this
does not affect functionality, it is very counter-intuitive. If a user
were to put the most likely matches at the top of the list to improve
performance, the effect would be a peformance hit. All other acl
elements are executed in their natural order.

Cause: use of list (lifo) structure for storing list elements.

Remedy: use stailq structure.

reschauzier <reschauzier@...> wrote:

> Allright, I got to the bottom of it. 
(many fixes)

Thank you for all the fixes. I will roll out 4.1.11 with it soon. What
name do you want to apear in the ChangeLog for your contribution, by the
way?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

> Thank you for all the fixes. I will roll out 4.1.11 with it soon. What
> name do you want to apear in the ChangeLog for your contribution, by the
> way?

My pleasure. Please use Rudy Eschauzier.