Milter-greylist

Hi,

	I think that I have found another bug in the MX synchronisation code.
The code checks that a peer is not the local server (function local_addr()
in sync.c) but the result of this test is only used in sync_sender().

	I think that this should be tested in sync_queue() as well.  With
the current code, sync_queue() adds pendings records to the list, even
if the peer is local and sync_sender() does not send the records and does
not clear the list.  Thus, the list grows as pending records are added and
an overflow occurs, e.g:
...
Dec  5 20:34:15 soleil milter-greylist: peer 193.51.24.1 queue overflow (1024 entries), discarding new entry
Dec  5 20:34:15 soleil milter-greylist: peer 193.51.24.1 queue overflow (1024 entries), discarding new entry
Dec  5 20:34:16 soleil milter-greylist: peer 193.51.24.1 queue overflow (1024 entries), discarding new entry
Dec  5 20:34:19 soleil milter-greylist: peer 193.51.24.1 queue overflow (1024 entries), discarding new entry
...

	I suggest to add a test at the beginning of sync_queue():

	if (peer->p_flags & P_LOCAL)
		return;

	This will prevent milter-greylist from queuing records to a local
peer.  And, BTW, this will fix a memory leak in milter-greylist (pending
records that are added to the local peer list are never deallocated since
their reference count will never be decremented).

	Comments, anyone?

	Cheers

		R\ufffdmy

Remy Card <Remy.Card@...> wrote:

>       I suggest to add a test at the beginning of sync_queue():
> 
>       if (peer->p_flags & P_LOCAL)
>               return;
> 
>       This will prevent milter-greylist from queuing records to a local
> peer.  And, BTW, this will fix a memory leak in milter-greylist (pending
> records that are added to the local peer list are never deallocated since
> their reference count will never be decremented).

The memory leak is limited because once the queue is full, it stops
growing.

Your analysis and your fix seem good to me. Did you tested it? Can I
commit it now? Should 1.6 include that fix? That will bound us to
another release candidate. Given that the problem is minor (a limited
chunk of ram held for nothing), I'd be in favor of only applying to the
developement branch and release 1.6 as is.
 
-- 
Emmanuel Dreyfus
Il y a 10 sortes de personnes dans le monde: ceux qui comprennent 
le binaire et ceux qui ne le comprennent pas.
manu@...

On Mon, Dec 06, 2004 at 02:22:21AM +0100, manu@... wrote:
> Remy Card <Remy.Card@...> wrote:
> >       I suggest to add a test at the beginning of sync_queue():
> > 
> >       if (peer->p_flags & P_LOCAL)
> >               return;
> > 
> >       This will prevent milter-greylist from queuing records to a local
> > peer.  And, BTW, this will fix a memory leak in milter-greylist (pending
> > records that are added to the local peer list are never deallocated since
> > their reference count will never be decremented).
> 
> The memory leak is limited because once the queue is full, it stops
> growing.

	Right.  But, then, your log files grow as overflow error messages are
generated when pending records are added to the peer sync list.

> Your analysis and your fix seem good to me. Did you tested it? Can I
> commit it now? Should 1.6 include that fix? That will bound us to
> another release candidate. Given that the problem is minor (a limited
> chunk of ram held for nothing), I'd be in favor of only applying to the
> developement branch and release 1.6 as is.

	I have tested this fix.  After applying it, overflow messages have
disappeared from my log files.  Anyway, since this is a minor bug, I vote
for applying the fix to the development branch if you don't want to release
another 1.6rc version.

		R\ufffdmy

This may be related. I find entries in the dumped file that contains 
the same tuple, with one
greylisted and the other autowhitelisted:


192.36.125.2    <FindRomance@...>    <someone@...>   
1102216438 # 2004-12-05 04:13:58
192.36.125.2    <FindRomance@...>    <someone@...>   
1103427423 AUTO # 2004-12-19 04:37:03
192.36.125.2    <Humbertoi7@...>      <someone@...>   
1102321606 # 2004-12-06 09:26:46
192.36.125.2    <Humbertoi7@...>      <someone@...>   
1103531697 AUTO # 2004-12-20 09:34:57


Shouldn't the tuples be unique in the memorybased database?