Milter-greylist

Hi,

I just tried to catch up on our NetBSD 4.0 (i386) box, upgraded from 4.1.10 to
4.3.3 and got this:

	# gdb /usr/pkg/bin/milter-greylist
	(...)
	(gdb) run
	Starting program: /usr/pkg/bin/milter-greylist
	(...)
	milter-greylist: Error detected by libpthread: Invalid condition variable.
	Detected by file "pthread_cond.c", line 283, function "pthread_cond_signal".
	See pthread(3) for information.

	Program received signal SIGABRT, Aborted.
	0xbbaf922f in kill () from /usr/lib/libc.so.12
	(gdb) bt
	#0  0xbbaf922f in kill () from /usr/lib/libc.so.12
	#1  0xbbbe3dcb in pthread__errorfunc () from /usr/lib/libpthread.so.0
	#2  0xbbbe36f3 in pthread_cond_signal () from /usr/lib/libpthread.so.0
	#3  0x080598d3 in dump_conf_changed ()
	#4  0x0805960a in conf_load_internal ()
	#5  0x080597bf in conf_load ()
	#6  0x0804dd87 in main ()

pthread_cond_signal takes {dump,sync}_sleepflag:

	# grep -r pthread_cond_signal milter-greylist-4.3.3
	#
	dump.c:        if ((error = pthread_cond_signal(&dump_sleepflag)) != 0) {
	dump.c:        pthread_cond_signal(&dump_sleepflag);
	dump.c:        pthread_cond_signal(&dump_sleepflag);
	sync.c:        if ((error = pthread_cond_signal(&sync_sleepflag)) != 0) {

	# egrep -r '(dump|sync)_sleepflag' milter-greylist-4.3.3
	#
	dump.c:static pthread_cond_t dump_sleepflag;
	dump.c:        if ((error = pthread_cond_init(&dump_sleepflag, NULL)) != 0) {
	dump.c:                                error = pthread_cond_timedwait(&dump_sleepflag,
	dump.c:                                error = pthread_cond_wait(&dump_sleepflag,
	dump.c:        if ((error = pthread_cond_signal(&dump_sleepflag)) != 0) {
	dump.c:        pthread_cond_signal(&dump_sleepflag);
	dump.c:        pthread_cond_signal(&dump_sleepflag);
	sync.c:static pthread_cond_t sync_sleepflag = PTHREAD_COND_INITIALIZER;
	sync.c:        if ((error = pthread_cond_signal(&sync_sleepflag)) != 0) {
	sync.c:                        pthread_cond_wait(&sync_sleepflag, &sync_dirty_lock);

Any ideas?



   Petar Bogdanovic

Petar Bogdanovic <petar@...> wrote:

> Any ideas?

First rebuild with -g to get the line number in dump.c: you'll know what
is the bad variable.

Then add printf before all operations on it, and you'll quickly find
what gors wrong.

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

... Attached patch should fix the problem.  Thanks for your hints! Petar Bogdanovic

Petar Bogdanovic <petar@...> wrote:

> Attached patch should fix the problem.  Thanks for your hints!

No patch attached...

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

On Tue, Sep 15, 2009 at 09:05:46PM +0200, Petar Bogdanovic wrote:
> On Sat, Sep 12, 2009 at 06:07:24AM +0200, manu@... wrote:
> > Petar Bogdanovic <petar@...> wrote:
> > 
> > > Any ideas?
> > 
> > First rebuild with -g to get the line number in dump.c: you'll know what
> > is the bad variable.
> > 
> > Then add printf before all operations on it, and you'll quickly find
> > what gors wrong.
> 
> Attached patch should fix the problem.

BTW:

   Until -r1.217 of milter-greylist.c, dump_sleepflag was initialized
   through pthread_cond_init(), called by dump_init().  That's why I
   didn't notice earlier.  dump_init() is in store.c now.

On Tue, Sep 15, 2009 at 09:43:52PM +0200, manu@... wrote:
> Petar Bogdanovic <petar@...> wrote:
> 
> > Attached patch should fix the problem.  Thanks for your hints!
> 
> No patch attached...

The Yahoo! Groups mailing-list software removes attachments.  Just
follow the link below and you'll see a list of all files originally
attached to my message:

On Tue, Sep 15, 2009 at 09:05:46PM +0200, Petar Bogdanovic wrote:
> ....Attachment(s) from Petar Bogdanovic included below.
>
> (...)
>
> ...Attachment(s) from Petar Bogdanovic:
>
>
> ... 1 of 1 File(s) http://groups.yahoo.com/group/milter-greylist/attachments/folder/121887736/item/list
>   ... cond.diff

Petar Bogdanovic <petar@...> wrote:

>    Until -r1.217 of milter-greylist.c, dump_sleepflag was initialized
>    through pthread_cond_init(), called by dump_init().  That's why I
>    didn't notice earlier.  dump_init() is in store.c now.

But with your patch you now initiliaze ir twice. Could'nt that cause
problems?

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

On Wed, Sep 16, 2009 at 04:36:10AM +0200, manu@... wrote:
> Petar Bogdanovic <petar@...> wrote:
> 
> >    Until -r1.217 of milter-greylist.c, dump_sleepflag was initialized
> >    through pthread_cond_init(), called by dump_init().  That's why I
> >    didn't notice earlier.  dump_init() is in store.c now.
> 
> But with your patch you now initiliaze ir twice. Could'nt that cause
> problems?

From what I've seen, no.  But OTOH I don't know much about libpthread.

But even if you want to do it right (through mg_init() which contains
dump_init() and is in store.c):  The problem is that you have a
chicken/egg situation here since conf_load() needs mg_init() (because of
dump_init()) and mg_init() needs conf_load() (because of the conf
struct).

I would suggest dropping pthread_cond_init() which is the only thing
dump_init() actually does.  man pthread_cond_init() says:

	``In cases where default condition variable attributes are
	  appropriate, the macro PTHREAD_COND_INITIALIZER can be used to
	  initialise condition variables that are statically allocated.
	  The effect is equivalent to dynamic initialisation by a call
	  to pthread_cond_init() with parameter attr specified as NULL,
	  except that no error checks are performed.''

Since dump.c already uses PTHREAD_MUTEX_INITIALIZER instead of
pthread_mutex_init(), this would be consistent.



   Petar Bogdanovic



P.S.
	Why am I the only person with this problem?

On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> 
> But even if you want to do it right (through mg_init() which contains
> dump_init() and is in store.c):  The problem is that you have a
> chicken/egg situation here since conf_load() needs mg_init() (because of
> dump_init()) and mg_init() needs conf_load() (because of the conf
> struct).
> 
> I would suggest dropping pthread_cond_init() which is the only thing
> dump_init() actually does.  man pthread_cond_init() says:
> 
> 	``In cases where default condition variable attributes are
> 	  appropriate, the macro PTHREAD_COND_INITIALIZER can be used to
> 	  initialise condition variables that are statically allocated.
> 	  The effect is equivalent to dynamic initialisation by a call
> 	  to pthread_cond_init() with parameter attr specified as NULL,
> 	  except that no error checks are performed.''
> 
> Since dump.c already uses PTHREAD_MUTEX_INITIALIZER instead of
> pthread_mutex_init(), this would be consistent.

Patch:

	http://smokva.net/patch/milter-greylist-cond.diff

I have been trying to wrap my head around this one for quite some time now. It sounds like moving dump_init() to mg_init() in store.c is what caused the problem? But since the program now calls mg_init() [which in its turn calls dump_init()] instead of dump_init(), shouldn't that leave the initialization of the condition variable unchanged? I must be overlooking something.


--- In milter-greylist@yahoogroups.com, Petar Bogdanovic <petar@...> wrote:

>    Until -r1.217 of milter-greylist.c, dump_sleepflag was initialized
>    through pthread_cond_init(), called by dump_init().  That's why I
>    didn't notice earlier.  dump_init() is in store.c now.
>

On Tue, Sep 29, 2009 at 08:12:31AM -0000, reschauzier wrote:
> I have been trying to wrap my head around this one for quite some time
> now. It sounds like moving dump_init() to mg_init() in store.c is what
> caused the problem? But since the program now calls mg_init() [which
> in its turn calls dump_init()] instead of dump_init(), shouldn't that
> leave the initialization of the condition variable unchanged? I must
> be overlooking something.

Yes, this:

On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> 
> But even if you want to do it right (through mg_init() which contains
> dump_init() and is in store.c):  The problem is that you have a
> chicken/egg situation here since conf_load() needs mg_init() (because of
> dump_init()) and mg_init() needs conf_load() (because of the conf
> struct).



   Petar Bogdanovic

Ah, I see. The problem is that mg_init() [and therefore dump_init()] is called _after_ conf_load() in main(). I am assuming that before the introduction of store.c, dump_init used to be called _before_ conf_load(), properly initializing the cond var. Is this what the problem was? Also, I wonder why nobody caught it before -- do you have any thoughts on that?

--- In milter-greylist@yahoogroups.com, Petar Bogdanovic <petar@...> wrote:

> 
> On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> > 
> > But even if you want to do it right (through mg_init() which contains
> > dump_init() and is in store.c):  The problem is that you have a
> > chicken/egg situation here since conf_load() needs mg_init() (because of
> > dump_init()) and mg_init() needs conf_load() (because of the conf
> > struct).
> 
> 
> 
>    Petar Bogdanovic
>

On Tue, Sep 29, 2009 at 09:59:46AM -0000, reschauzier wrote:
> Ah, I see. The problem is that mg_init() [and therefore dump_init()]
> is called _after_ conf_load() in main(). I am assuming that before the
> introduction of store.c, dump_init used to be called _before_
> conf_load(), properly initializing the cond var. Is this what the
> problem was?

Yes.


> Also, I wonder why nobody caught it before -- do you have any thoughts
> on that?

I guess it depends on your local pthreads implementation.  The NetBSD
libc SIGABRTs the process if cond wasn't initialized properly:

	| __RCSID("$NetBSD: pthread_cond.c,v 1.18 2005/01/06 17:33:36 mycroft Exp $");
	|
	| (...)
	|
	| int
	| pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr)
	| {
	|
	| 	pthread__error(EINVAL, "Invalid condition variable attribute",
	| 	    (attr == NULL) || (attr->ptca_magic == _PT_CONDATTR_MAGIC));

glibc OTOH doesn't care (see nptl/pthread_cond_init.c).



   Petar Bogdanovic

Petar Bogdanovic <petar@...> wrote:

> On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> > But even if you want to do it right (through mg_init() which contains
> > dump_init() and is in store.c):  The problem is that you have a
> > chicken/egg situation here since conf_load() needs mg_init() (because of
> > dump_init()) and mg_init() needs conf_load() (because of the conf
> > struct).

The fix is in the lastest release, isn't it? 

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

On Tue, Sep 29, 2009 at 02:20:24PM +0200, manu@... wrote:
> Petar Bogdanovic <petar@...> wrote:
> 
> > On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> > > But even if you want to do it right (through mg_init() which contains
> > > dump_init() and is in store.c):  The problem is that you have a
> > > chicken/egg situation here since conf_load() needs mg_init() (because of
> > > dump_init()) and mg_init() needs conf_load() (because of the conf
> > > struct).
> 
> The fix is in the lastest release, isn't it?

Yes, it's all good.  I just replied to Rudys follow-up.

Just wondered if the introduction of store.c messed something up. It looks like it did. Thanks to Petar for catching and fixing!

--- In milter-greylist@yahoogroups.com, Petar Bogdanovic <petar@...> wrote:

>
> On Tue, Sep 29, 2009 at 02:20:24PM +0200, manu@... wrote:
> > Petar Bogdanovic <petar@...> wrote:
> > 
> > > On Wed, Sep 16, 2009 at 11:04:33AM +0200, Petar Bogdanovic wrote:
> > > > But even if you want to do it right (through mg_init() which contains
> > > > dump_init() and is in store.c):  The problem is that you have a
> > > > chicken/egg situation here since conf_load() needs mg_init() (because of
> > > > dump_init()) and mg_init() needs conf_load() (because of the conf
> > > > struct).
> > 
> > The fix is in the lastest release, isn't it?
> 
> Yes, it's all good.  I just replied to Rudys follow-up.
>