Re: nlif_open() - nlif_close() - memory leak?

Re: nlif_open() - nlif_close() - memory leak?

[Anton]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]


> The attached patch should fix the memory leak issue.
> Could you test it ?

Accordingly to "top", it's still leaking.

We used modified libnfnetlink-0.0.33/utils/iftest.c 
(attached)

THe following compile line was used:
gcc -o  iftest  iftest.c  -lnetfilter_queue -lnfnetlink

-------------------------------------------------------

[-- Attachment #2: iftest.c --]
[-- Type: text/x-csrc, Size: 646 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

#include <libnfnetlink/libnfnetlink.h>

#include <signal.h>

static int TERM=0;

void sig_term(int s) {
 TERM=1;
}

int main() {
        int i;
        struct nlif_handle *h;

        signal(SIGTERM,sig_term);
        signal(SIGINT,sig_term);

         while(!TERM) {
            h = nlif_open();
            if (h == NULL) {
                perror("nlif_open");
                exit(EXIT_FAILURE);
            }

            nlif_query(h);
	    usleep(1000);
            nlif_close(h);
        }

        signal(SIGTERM,SIG_DFL);
        signal(SIGINT,SIG_DFL);

        puts("Bye");
}

Re: nlif_open() - nlif_close() - memory leak?

[Eric Leblond]

[-- Attachment #1.1: Type: text/plain, Size: 376 bytes --]

Hello,

On Thursday, 2008 May 22 at 15:00:35 +0500, Anton wrote:
> 
> > The attached patch should fix the memory leak issue.
> > Could you test it ?
> 
> Accordingly to "top", it's still leaking.

Yes, I should not code during conferences ;)

I attach a patch which is working for me.

BR,
-- 
Eric Leblond
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[-- Attachment #1.2: fix_nlif_memleak.patch --]
[-- Type: text/x-diff, Size: 1497 bytes --]

diff --git a/src/iftable.c b/src/iftable.c
index 7e6172b..01ccc8c 100644
--- a/src/iftable.c
+++ b/src/iftable.c
@@ -25,6 +25,8 @@
 
 #define iftb_log(x, ...)
 
+#define SIZE_IFHASH 16
+
 struct ifindex_map {
 	struct ifindex_map *next;
 
@@ -37,7 +39,7 @@ struct ifindex_map {
 };
 
 struct nlif_handle {
-	struct ifindex_map *ifindex_map[16];
+	struct ifindex_map *ifindex_map[SIZE_IFHASH];
 	struct rtnl_handle *rtnl_handle;
 	struct rtnl_handler ifadd_handler;
 	struct rtnl_handler ifdel_handler;
@@ -222,6 +224,7 @@ static int iftable_up(struct nlif_handle *nlif_handle, unsigned int index)
 struct nlif_handle *nlif_open(void)
 {
 	struct nlif_handle *h;
+	int i;
 
 	h = calloc(1,  sizeof(struct nlif_handle));
 	if (h == NULL)
@@ -255,6 +258,21 @@ err:
 	return NULL;
 }
 
+static void free_indexmap(struct ifindex_map **indexmap)
+{
+	struct ifindex_map *ima, *imp;
+	int i;
+
+	for (i = 0; i < SIZE_IFHASH; i++) {
+		for (ima = indexmap[i]; ima != NULL; ima = imp) {
+			if (ima) {
+				imp = ima->next;
+				free(ima);
+			}
+		}
+	}
+}
+
 /** Destructor of interface table
  *
  * \param nlif_handle A pointer to a ::nlif_handle created 
@@ -267,6 +285,7 @@ void nlif_close(struct nlif_handle *h)
 	rtnl_handler_unregister(h->rtnl_handle, &h->ifadd_handler);
 	rtnl_handler_unregister(h->rtnl_handle, &h->ifdel_handler);
 	rtnl_close(h->rtnl_handle);
+	free_indexmap(h->ifindex_map);
 	free(h);
 	h = NULL; /* bugtrap */
 }

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: nlif_open() - nlif_close() - memory leak?

[Pablo Neira Ayuso]

Eric Leblond wrote:
> Hello,
> 
> On Thursday, 2008 May 22 at 15:00:35 +0500, Anton wrote:
>>> The attached patch should fix the memory leak issue.
>>> Could you test it ?
>> Accordingly to "top", it's still leaking.
> 
> Yes, I should not code during conferences ;)
> 
> I attach a patch which is working for me.

Oh, I didn't notice this patch. I think that my patch results in a
cleaner code. The current list handling is a bit error prone. What do
you think?

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: nlif_open() - nlif_close() - memory leak?

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

Hello again,

On Thursday, 2008 May 22 at 15:08:12 +0200, Pablo Neira Ayuso wrote:
> Eric Leblond wrote:
> > Hello,
> > 
> > I attach a patch which is working for me.
> 
> Oh, I didn't notice this patch. I think that my patch results in a
> cleaner code. The current list handling is a bit error prone. What do
> you think?

I'm not really found of fixing it with a huge patch because libnfnetlink
code is currently stable and because the code should not evolve much
(All features are implemented and base netlink implementation is old and
stable).

BR,
-- 
Eric Leblond
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

nlif_open() - nlif_close() - memory leak?

[Anton]

Hello!

Just doing some testuing of NFQUEUE, and noticed that in the 
simple case, if I do the following (yes, there is 
no "nlif_catch", but anyway) - I'm experiencing a memory 
leak for every open - close cycle?
Anything I'm missing?

       struct nlif_handle *h;
        h = nlif_open();
        if (h != NULL) {
         nlif_query(h);
         nfq_get_indev_name(h,tb,agg.iname);
         nfq_get_outdev_name(h,tb,agg.oname);
         nlif_close(h);
        }

Re: nlif_open() - nlif_close() - memory leak?

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

Hello,

On Thursday, 2008 May 22 at 10:53:40 +0500, Anton wrote:
> Hello!
> 
> Just doing some testuing of NFQUEUE, and noticed that in the 
> simple case, if I do the following (yes, there is 
> no "nlif_catch", but anyway) - I'm experiencing a memory 
> leak for every open - close cycle?
> Anything I'm missing?
> 
>        struct nlif_handle *h;
>         h = nlif_open();
>         if (h != NULL) {
>          nlif_query(h);
>          nfq_get_indev_name(h,tb,agg.iname);
>          nfq_get_outdev_name(h,tb,agg.oname);
>          nlif_close(h);
>         }

You should not call nlif_open, nlif_close at each resolution:

Information about nlif usage:
http://software.inl.fr/trac/wiki/articles/using_nlif

Anyway, I will investigate to see what's going on. We definitely should
not have a memory leak.

BR,
-- 
Eric Leblond
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: nlif_open() - nlif_close() - memory leak?

[Anton]

On Thursday 22 May 2008 12:43, Eric Leblond wrote:
> Hello,
>
> On Thursday, 2008 May 22 at 10:53:40 +0500, Anton wrote:
> You should not call nlif_open, nlif_close at each
> resolution:

Yes, this is obvious, it was just a quick test.

>
> Information about nlif usage:
> http://software.inl.fr/trac/wiki/articles/using_nlif

Great, thanks!

>
> Anyway, I will investigate to see what's going on. We
> definitely should not have a memory leak.
>
> BR,

Re: nlif_open() - nlif_close() - memory leak?

[Eric Leblond]

[-- Attachment #1.1: Type: text/plain, Size: 722 bytes --]

Hello,

On Thursday, 2008 May 22 at 12:52:41 +0500, Anton wrote:
> On Thursday 22 May 2008 12:43, Eric Leblond wrote:
> > Hello,
> >
> > On Thursday, 2008 May 22 at 10:53:40 +0500, Anton wrote:
> > You should not call nlif_open, nlif_close at each
> > resolution:
> 
> Yes, this is obvious, it was just a quick test.
> 
> >
> > Information about nlif usage:
> > http://software.inl.fr/trac/wiki/articles/using_nlif
> 
> Great, thanks!
> 
> >
> > Anyway, I will investigate to see what's going on. We
> > definitely should not have a memory leak.

The attached patch should fix the memory leak issue. Could you test it ?

BR,
-- 
Eric Leblond
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[-- Attachment #1.2: memleak.patch --]
[-- Type: text/x-diff, Size: 1254 bytes --]

diff --git a/src/iftable.c b/src/iftable.c
index 7e6172b..a2844fd 100644
--- a/src/iftable.c
+++ b/src/iftable.c
@@ -25,6 +25,8 @@
 
 #define iftb_log(x, ...)
 
+#define SIZE_IFHASH 16
+
 struct ifindex_map {
 	struct ifindex_map *next;
 
@@ -37,7 +39,7 @@ struct ifindex_map {
 };
 
 struct nlif_handle {
-	struct ifindex_map *ifindex_map[16];
+	struct ifindex_map *ifindex_map[SIZE_IFHASH];
 	struct rtnl_handle *rtnl_handle;
 	struct rtnl_handler ifadd_handler;
 	struct rtnl_handler ifdel_handler;
@@ -255,6 +257,19 @@ err:
 	return NULL;
 }
 
+static void free_indexmap(struct ifindex_map **indexmap)
+{
+	struct ifindex_map *im, *ima, **imp;
+	int i;
+
+	for (i = 0; i < SIZE_IFHASH; i++) {
+		for (ima = NULL, imp = &(indexmap[i]); 
+				(im=*imp)!=NULL; imp = &im->next, ima=im) {
+			free(ima);
+		}
+	}
+}
+
 /** Destructor of interface table
  *
  * \param nlif_handle A pointer to a ::nlif_handle created 
@@ -267,6 +282,7 @@ void nlif_close(struct nlif_handle *h)
 	rtnl_handler_unregister(h->rtnl_handle, &h->ifadd_handler);
 	rtnl_handler_unregister(h->rtnl_handle, &h->ifdel_handler);
 	rtnl_close(h->rtnl_handle);
+	free_indexmap(h->ifindex_map);
 	free(h);
 	h = NULL; /* bugtrap */
 }

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]