Mailing List Archive: SPF: Devel

Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak?

Index | Next | Previous | View Threaded

pettai at nordu

Oct 24, 2008, 9:02 AM

Post #1 of 9 (4621 views)
Permalink

Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak?

Hi,

According to tests done by some people in the milter-greylist community, libspf2
(and possible libspf_alt) causes memory leaks on some OSes. See a cut version
from the posts below:

>On Fri, Oct 24, 2008 at 02:07:58AM +0900, Hajimu UMEMOTO wrote:
>>>>>> On Thu, 23 Oct 2008 14:31:15 +0200
>>>>>> Petar Bogdanovic <petar [at] smokva> said:
>
> petar> The situation even got worse with libspf2 from another point of view
> petar> -- milter-greylist using libspf_alt claimed about 15% of the CPU when
> petar> it had to deal with 900 simultaneous connections while the same
> petar> situation ate every second of the available CPU-time when linked
> petar> against libspf2. That's why I wasn't able to drop the same amounth
> petar> of mails through milter-greylist+libspf2: the CPU was the
> petar> bottleneck.
>
>> The BIND9's resolver requires res_ndestroy() for cleanup res_state.
>> However, libspf2 doesn't issue res_ndestroy() but issues res_nclose().
>> It causes memory leak. The FreeBSD port of libspf2 has a patch to
>> address this issue.
>
>Thanks, I applied all patches (~10) from the FreeBSD Ports tree and the
>problem is gone. The memory usage of milter-greylist is actually frozen
>now, no matter how hard I try. Also, the claimed cpu-time is much lower.

This has been fixed by a patch in the FreeBSD Ports tree. See (and specifically
patch submission [2]):
ftp://ftp.freebsd.org/pub/FreeBSD/development/FreeBSD-CVS/ports/mail/libspf2/files/patch-src_libspf2_spf__dns__resolv.c,v

AFAICS, this fix doesn't seem to be in the latest version of the libspf2-1.2.8
source. Maybe someone can add it to libspf2's main CVS branch instead?

(Someone mentioned that this also seems to be the case in the older (and perhaps
unmaintained libspf_alt, but I don't know if anyone cared to test that).

Regards,
Fredrik

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

scott at kitterman

Oct 24, 2008, 9:30 AM

Post #2 of 9 (4475 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

On Friday 24 October 2008 12:02, Fredrik Pettai wrote:

> AFAICS, this fix doesn't seem to be in the latest version of the
> libspf2-1.2.8 source. Maybe someone can add it to libspf2's main CVS branch
> instead?
>

For reasons I'm not quite sure of since I don't do C code, I have access to
the libspf2 svn. I looked and this is at least partially done in SVN, but
also some of the code has been refactored, so I'm not sure the rest is
applicable. I'll make sure the maintainer is aware of the patch.

Scott K

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

ume at mahoroba

Oct 24, 2008, 9:48 AM

Post #3 of 9 (4478 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

Hi,

>>>>> On Fri, 24 Oct 2008 18:02:36 +0200
>>>>> Fredrik Pettai <pettai [at] nordu> said:

pettai> This has been fixed by a patch in the FreeBSD Ports tree. See (and specifically
pettai> patch submission [2]):
pettai> ftp://ftp.freebsd.org/pub/FreeBSD/development/FreeBSD-CVS/ports/mail/libspf2/files/patch-src_libspf2_spf__dns__resolv.c,v

pettai> AFAICS, this fix doesn't seem to be in the latest version of the libspf2-1.2.8
pettai> source. Maybe someone can add it to libspf2's main CVS branch instead?

Here is a patch against 1.2.8:

Index: src/libspf2/spf_dns_resolv.c
diff -u -p src/libspf2/spf_dns_resolv.c.orig src/libspf2/spf_dns_resolv.c
--- src/libspf2/spf_dns_resolv.c.orig Thu Oct 16 07:02:03 2008
+++ src/libspf2/spf_dns_resolv.c Fri Oct 24 12:19:29 2008
@@ -92,7 +92,11 @@ static pthread_key_t res_state_key;
static void
SPF_dns_resolv_thread_term(void *arg)
{
+#ifdef res_ndestroy
+ res_ndestroy( (struct __res_state *)arg );
+#else
res_nclose( (struct __res_state *)arg );
+#endif
free(arg);
}

@@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
#if HAVE_DECL_RES_NINIT
pthread_once(&res_state_control, SPF_dns_resolv_init_key);
#else
- if ( res_init() != 0 ) {
+ if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
perror("res_init");
return NULL;
}

Sincerely,

--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
ume [at] mahoroba ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

hannah at schlund

Oct 27, 2008, 6:52 AM

Post #4 of 9 (4455 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

Hi!

On Sat, Oct 25, 2008 at 01:48:32AM +0900, Hajimu UMEMOTO wrote:
>>>>>> On Fri, 24 Oct 2008 18:02:36 +0200
>>>>>> Fredrik Pettai <pettai [at] nordu> said:

>pettai> This has been fixed by a patch in the FreeBSD Ports tree. See (and specifically
>pettai> patch submission [2]):
>pettai> ftp://ftp.freebsd.org/pub/FreeBSD/development/FreeBSD-CVS/ports/mail/libspf2/files/patch-src_libspf2_spf__dns__resolv.c,v

>pettai> AFAICS, this fix doesn't seem to be in the latest version of the libspf2-1.2.8
>pettai> source. Maybe someone can add it to libspf2's main CVS branch instead?

>Here is a patch against 1.2.8:

>Index: src/libspf2/spf_dns_resolv.c
>diff -u -p src/libspf2/spf_dns_resolv.c.orig src/libspf2/spf_dns_resolv.c
>--- src/libspf2/spf_dns_resolv.c.orig Thu Oct 16 07:02:03 2008
>+++ src/libspf2/spf_dns_resolv.c Fri Oct 24 12:19:29 2008
>@@ -92,7 +92,11 @@ static pthread_key_t res_state_key;
> static void
> SPF_dns_resolv_thread_term(void *arg)
> {
>+#ifdef res_ndestroy
>+ res_ndestroy( (struct __res_state *)arg );
>+#else
> res_nclose( (struct __res_state *)arg );
>+#endif
> free(arg);
> }

I see this, but...

>@@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
> #if HAVE_DECL_RES_NINIT
> pthread_once(&res_state_control, SPF_dns_resolv_init_key);
> #else
>- if ( res_init() != 0 ) {
>+ if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
> perror("res_init");
> return NULL;
> }

Why is that needed, and how portable is that in fact, compared to
calling res_init unconditionally in that place?

Kind regards,

Hannah.

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

johann+spfdevel at klasek

Oct 27, 2008, 7:15 PM

Post #5 of 9 (4449 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

On Sat, Oct 25, 2008 at 01:48:32AM +0900, Hajimu UMEMOTO wrote:
> Hi,
>
> >>>>> On Fri, 24 Oct 2008 18:02:36 +0200
> >>>>> Fredrik Pettai <pettai [at] nordu> said:
>
> pettai> This has been fixed by a patch in the FreeBSD Ports tree. See (and specifically
> pettai> patch submission [2]):
> pettai> ftp://ftp.freebsd.org/pub/FreeBSD/development/FreeBSD-CVS/ports/mail/libspf2/files/patch-src_libspf2_spf__dns__resolv.c,v
>
> pettai> AFAICS, this fix doesn't seem to be in the latest version of the libspf2-1.2.8
> pettai> source. Maybe someone can add it to libspf2's main CVS branch instead?
>
> Here is a patch against 1.2.8:
>
> Index: src/libspf2/spf_dns_resolv.c
> diff -u -p src/libspf2/spf_dns_resolv.c.orig src/libspf2/spf_dns_resolv.c
> --- src/libspf2/spf_dns_resolv.c.orig Thu Oct 16 07:02:03 2008
> +++ src/libspf2/spf_dns_resolv.c Fri Oct 24 12:19:29 2008
> @@ -92,7 +92,11 @@ static pthread_key_t res_state_key;
> static void
> SPF_dns_resolv_thread_term(void *arg)
> {
> +#ifdef res_ndestroy
> + res_ndestroy( (struct __res_state *)arg );
> +#else
> res_nclose( (struct __res_state *)arg );
> +#endif
> free(arg);
> }
>
> @@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
> #if HAVE_DECL_RES_NINIT
> pthread_once(&res_state_control, SPF_dns_resolv_init_key);
> #else
> - if ( res_init() != 0 ) {
> + if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
> perror("res_init");
> return NULL;
> }

Sorry, but this patch won't work on platform where res_* functions are
real library functions, e.g on Solaris at least up to Solaris 9 ... The
above fix is targeting mainly Linux or maybe NetBSD where res_* is just
a macro layer. Even with checking the __RES macro value (resolver
release date) this could not be catched (e.g. Solaris 9 has
res_ndestroy, Solaris 8 not but both have the same __RES value!).

AFIAK some autoconf stuff is needed to get the information if
res_ndestroy can (should) be used!

Johann Klasek

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

hannah at schlund

Oct 28, 2008, 6:27 AM

Post #6 of 9 (4453 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

Hi!

On Tue, Oct 28, 2008 at 03:15:20AM +0100, Johann E. Klasek wrote:
>On Sat, Oct 25, 2008 at 01:48:32AM +0900, Hajimu UMEMOTO wrote:
>> >>>>> On Fri, 24 Oct 2008 18:02:36 +0200
>> >>>>> Fredrik Pettai <pettai [at] nordu> said:

>> pettai> This has been fixed by a patch in the FreeBSD Ports tree. See (and specifically
>> pettai> patch submission [2]):
>> pettai> ftp://ftp.freebsd.org/pub/FreeBSD/development/FreeBSD-CVS/ports/mail/libspf2/files/patch-src_libspf2_spf__dns__resolv.c,v

>> pettai> AFAICS, this fix doesn't seem to be in the latest version of the libspf2-1.2.8
>> pettai> source. Maybe someone can add it to libspf2's main CVS branch instead?

>> Here is a patch against 1.2.8:

>> Index: src/libspf2/spf_dns_resolv.c
>> diff -u -p src/libspf2/spf_dns_resolv.c.orig src/libspf2/spf_dns_resolv.c
>> --- src/libspf2/spf_dns_resolv.c.orig Thu Oct 16 07:02:03 2008
>> +++ src/libspf2/spf_dns_resolv.c Fri Oct 24 12:19:29 2008
>> @@ -92,7 +92,11 @@ static pthread_key_t res_state_key;
>> static void
>> SPF_dns_resolv_thread_term(void *arg)
>> {
>> +#ifdef res_ndestroy
>> + res_ndestroy( (struct __res_state *)arg );
>> +#else
>> res_nclose( (struct __res_state *)arg );
>> +#endif
>> free(arg);
>> }

>> @@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
>> #if HAVE_DECL_RES_NINIT
>> pthread_once(&res_state_control, SPF_dns_resolv_init_key);
>> #else
>> - if ( res_init() != 0 ) {
>> + if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
>> perror("res_init");
>> return NULL;
>> }

>Sorry, but this patch won't work on platform where res_* functions are
>real library functions, e.g on Solaris at least up to Solaris 9 ... The
>above fix is targeting mainly Linux or maybe NetBSD where res_* is just
>a macro layer. Even with checking the __RES macro value (resolver
>release date) this could not be catched (e.g. Solaris 9 has
>res_ndestroy, Solaris 8 not but both have the same __RES value!).

>AFIAK some autoconf stuff is needed to get the information if
>res_ndestroy can (should) be used!

Makes sense.

>Johann Klasek

Kind regards,

Hannah.

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

hannah at schlund

Oct 28, 2008, 6:40 AM

Post #7 of 9 (4463 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

Hi!

On Tue, Oct 28, 2008 at 02:27:28PM +0100, Hannah Schroeter wrote:
>On Tue, Oct 28, 2008 at 03:15:20AM +0100, Johann E. Klasek wrote:
>>On Sat, Oct 25, 2008 at 01:48:32AM +0900, Hajimu UMEMOTO wrote:
>[...]

>>AFIAK some autoconf stuff is needed to get the information if
>>res_ndestroy can (should) be used!

>Makes sense.

And I see that in the svn repository it has been done just that way
(with autoconf), for that one first diff chunk.

Kind regards,

Hannah.

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

ume at mahoroba

Oct 29, 2008, 10:39 AM

Post #8 of 9 (4453 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

Hi,

>>>>> On Mon, 27 Oct 2008 14:52:15 +0100
>>>>> Hannah Schroeter <hannah [at] schlund> said:

>@@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
> #if HAVE_DECL_RES_NINIT
> pthread_once(&res_state_control, SPF_dns_resolv_init_key);
> #else
>- if ( res_init() != 0 ) {
>+ if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
> perror("res_init");
> return NULL;
> }

hannah> Why is that needed, and how portable is that in fact, compared to
hannah> calling res_init unconditionally in that place?

Because, the libspf2 is not only consumer of the resolver. The
res_init() might be called already before calling it from libspf2. It
is enough to call res_init() once.
Basically, res_init() is used in this manner on BSDs. The impact of
calling res_init() again and again is unclear.
I'm not sure about portability, but the _res and RES_INIT is described
in resolver(8) on at least BSDs and CentOS.

Sincerely,

--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
ume [at] mahoroba ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

spf at anarres

Nov 4, 2008, 8:31 AM

Post #9 of 9 (4419 views)
Permalink

Re: Fwd: [milter-greylist] HEADS-UP: libspf2 causes milter-greylist memory leak? [In reply to]

On Thu, 2008-10-30 at 02:39 +0900, Hajimu UMEMOTO wrote:
> Hi,
>
> >>>>> On Mon, 27 Oct 2008 14:52:15 +0100
> >>>>> Hannah Schroeter <hannah [at] schlund> said:
>
> >@@ -615,7 +619,7 @@ SPF_dns_resolv_new(SPF_dns_server_t *lay
> > #if HAVE_DECL_RES_NINIT
> > pthread_once(&res_state_control, SPF_dns_resolv_init_key);
> > #else
> >- if ( res_init() != 0 ) {
> >+ if ((_res.options & RES_INIT) == 0 && res_init() != 0) {
> > perror("res_init");
> > return NULL;
> > }
>
> hannah> Why is that needed, and how portable is that in fact, compared to
> hannah> calling res_init unconditionally in that place?
>
> Because, the libspf2 is not only consumer of the resolver. The
> res_init() might be called already before calling it from libspf2. It
> is enough to call res_init() once.
> Basically, res_init() is used in this manner on BSDs. The impact of
> calling res_init() again and again is unclear.
> I'm not sure about portability, but the _res and RES_INIT is described
> in resolver(8) on at least BSDs and CentOS.

I do not believe we are allowed to look inside _res. I have certainly
included the patch for res_ndestroy, but I believe the library authors
have to make calling res_init() multiple times safe. Perhaps you could
check this in your system source code? I have checked two
implementations, including the one at
http://www.koders.com/c/fid6F05A2EABB6A6CFB02D691F0F98DFB676F0B15E7.aspx?s=sort#L212 and both appear safe.

Please see 1.2.9. Further patches will make 1.2.10 in a week or two.

S.

-------------------------------------------
Sender Policy Framework: http://www.openspf.org
Modify Your Subscription: http://www.listbox.com/member/
Archives: https://www.listbox.com/member/archive/1007/=now
RSS Feed: https://www.listbox.com/member/archive/rss/1007/
Powered by Listbox: http://www.listbox.com

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads