libiptc memory leak - PATCH

libiptc memory leak - PATCH

[Tomáš Lejdar]

Hi,
I've used libiptc for some utils and found a little
problem with memory allocation, discused in January and
may be earlier. Here is a small patch (r.1.2.8):

*** libiptc/libiptc.c.orig      Mon Apr 21 23:54:48 2003
--- libiptc/libiptc.c   Mon Apr 21 23:59:30 2003
***************
*** 504,511 ****
         (*handle)->cache_chain_iteration++;

         if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
!           == (*handle)->cache_num_chains)
                 return NULL;

         return (*handle)->cache_chain_iteration->name;
   }
--- 504,513 ----
         (*handle)->cache_chain_iteration++;

         if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
!           == (*handle)->cache_num_chains){
!                 free((*handle)->cache_chain_heads);
                 return NULL;
+       }

         return (*handle)->cache_chain_iteration->name;
   }

T.v.L

Re: libiptc memory leak - PATCH

[Harald Welte]

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

On Tue, Apr 22, 2003 at 12:25:34AM +0200, Tomá? Lejdar wrote:
> Hi,
> I've used libiptc for some utils and found a little
> problem with memory allocation, discused in January and
> may be earlier. Here is a small patch (r.1.2.8):

thanks!

please send unified diffs (diff -u) in the future.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: libiptc memory leak - PATCH

[Harald Welte]

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

On Tue, Apr 22, 2003 at 12:25:34AM +0200, Tomá? Lejdar wrote:
>         (*handle)->cache_chain_iteration++;
> 
>         if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
> !           == (*handle)->cache_num_chains){
> !                 free((*handle)->cache_chain_heads);
>                 return NULL;
> +       }
> 
>         return (*handle)->cache_chain_iteration->name;
>   }

I cannot accept this patch It would basically mean that something like:

first_chain()
next_chain()
first_chain()

would no longer work.

we need an explicit free_handle that cleans up all allocated regions
instead.  Patches are welcome.

> T.v.L

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: libiptc memory leak - PATCH

[Tomáš Lejdar]

Hi,
I thing there is no problem, the memory is freed only when the
function next_chain returns NULL and run through the chains is on the end.
In the next call of first_chain() is memory alocated again in 
populate_cache().

Is there something wrong in my idea ?
T.v.L


Harald Welte wrote:
I cannot accept this patch It would basically mean that something like:

>first_chain()
>next_chain()
>first_chain()
>
>would no longer work.
>
>we need an explicit free_handle that cleans up all allocated regions
>instead.  Patches are welcome.
>  
>

Re: libiptc memory leak - PATCH

[Harald Welte]

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

On Mon, Apr 28, 2003 at 09:14:25AM +0200, Tomá? Lejdar wrote:
> Hi,
> I thing there is no problem, the memory is freed only when the
> function next_chain returns NULL and run through the chains is on the end.
> In the next call of first_chain() is memory alocated again in 
> populate_cache().

no, it's fine.  It was a misunderstanding. I'll apply your patch now.
Sorry for the fuzz.

> T.v.L

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: libiptc memory leak - PATCH

[Martin Josefsson]

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

On Wed, 2003-04-30 at 17:47, Harald Welte wrote:
> On Mon, Apr 28, 2003 at 09:14:25AM +0200, Tomá? Lejdar wrote:
> > Hi,
> > I thing there is no problem, the memory is freed only when the
> > function next_chain returns NULL and run through the chains is on the end.
> > In the next call of first_chain() is memory alocated again in 
> > populate_cache().
> 
> no, it's fine.  It was a misunderstanding. I'll apply your patch now.
> Sorry for the fuzz.

This patch breaks {ip,ip6}tables-{save,restore} and it runs free() on
the wrong memoryaddress (I think free() does the right thing anyway but
the patch still breaks stuff).

-{save,restore} segfaulted with the patch.

I've attached a patch that reverts this patch and adds
iptc_exit(iptc_handle_t h) that takes care of the freeing.
It also contains some small cleanups.

The patch also fixes iptables, ip6tables and their -{save,restore}
cousins to not leak memory.

I've tested iptables and iptables-{save,restore} and they appear to work
just fine.

Please apply or at least revert the previous patch.

-- 
/Martin

[-- Attachment #2: libiptc-diff --]
[-- Type: text/x-patch, Size: 7268 bytes --]

diff -x '*.d' -urN userspace.orig/include/libiptc/libiptc.h userspace/include/libiptc/libiptc.h
--- userspace.orig/include/libiptc/libiptc.h	2001-04-19 18:35:39.000000000 +0200
+++ userspace/include/libiptc/libiptc.h	2003-05-02 13:48:01.000000000 +0200
@@ -34,6 +34,9 @@
 /* Take a snapshot of the rules.  Returns NULL on error. */
 iptc_handle_t iptc_init(const char *tablename);
 
+/* Cleanup after iptc_init(). */
+void iptc_exit(iptc_handle_t h);
+
 /* Iterator functions to run through the chains.  Returns NULL at end. */
 const char *iptc_first_chain(iptc_handle_t *handle);
 const char *iptc_next_chain(iptc_handle_t *handle);
diff -x '*.d' -urN userspace.orig/ip6tables-restore.c userspace/ip6tables-restore.c
--- userspace.orig/ip6tables-restore.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/ip6tables-restore.c	2003-05-02 13:58:33.000000000 +0200
@@ -102,7 +102,7 @@
 
 int main(int argc, char *argv[])
 {
-	ip6tc_handle_t handle;
+	ip6tc_handle_t handle = NULL;
 	char buffer[10240];
 	int c;
 	char curtable[IP6T_TABLE_MAXNAMELEN + 1];
@@ -183,6 +183,9 @@
 			}
 			strncpy(curtable, table, IP6T_TABLE_MAXNAMELEN);
 
+			if (handle)
+				ip6tc_exit(handle);
+
 			handle = create_handle(table, modprobe);
 			if (noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
diff -x '*.d' -urN userspace.orig/ip6tables-save.c userspace/ip6tables-save.c
--- userspace.orig/ip6tables-save.c	2002-08-14 13:40:41.000000000 +0200
+++ userspace/ip6tables-save.c	2003-05-02 13:50:01.000000000 +0200
@@ -305,6 +305,8 @@
 		exit_error(OTHER_PROBLEM, "Binary NYI\n");
 	}
 
+	ip6tc_exit(h);
+
 	return 1;
 }
 
diff -x '*.d' -urN userspace.orig/ip6tables.c userspace/ip6tables.c
--- userspace.orig/ip6tables.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/ip6tables.c	2003-05-02 14:34:23.000000000 +0200
@@ -1670,6 +1670,7 @@
 	const char *modprobe = NULL;
 	int proto_used = 0;
 	char icmp6p[] = "icmpv6";
+	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2147,8 +2148,10 @@
 			   chain, IP6T_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle)
+	if (!*handle) {
 		*handle = ip6tc_init(*table);
+		no_handle = 1;
+	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2293,5 +2296,10 @@
 	if (verbose > 1)
 		dump_entries6(*handle);
 
+	if (no_handle) {
+		ip6tc_exit(*handle);
+		*handle = NULL;
+	}
+
 	return ret;
 }
diff -x '*.d' -urN userspace.orig/iptables-restore.c userspace/iptables-restore.c
--- userspace.orig/iptables-restore.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/iptables-restore.c	2003-05-02 14:13:15.000000000 +0200
@@ -99,7 +99,7 @@
 
 int main(int argc, char *argv[])
 {
-	iptc_handle_t handle;
+	iptc_handle_t handle = NULL;
 	char buffer[10240];
 	int c;
 	char curtable[IPT_TABLE_MAXNAMELEN + 1];
@@ -180,6 +180,9 @@
 			}
 			strncpy(curtable, table, IPT_TABLE_MAXNAMELEN);
 
+			if (handle)
+				iptc_exit(handle);
+
 			handle = create_handle(table, modprobe);
 			if (noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
diff -x '*.d' -urN userspace.orig/iptables-save.c userspace/iptables-save.c
--- userspace.orig/iptables-save.c	2002-08-07 11:07:41.000000000 +0200
+++ userspace/iptables-save.c	2003-05-02 13:49:37.000000000 +0200
@@ -304,6 +304,8 @@
 		exit_error(OTHER_PROBLEM, "Binary NYI\n");
 	}
 
+	iptc_exit(h);
+
 	return 1;
 }
 
diff -x '*.d' -urN userspace.orig/iptables.c userspace/iptables.c
--- userspace.orig/iptables.c	2003-03-31 14:33:55.000000000 +0200
+++ userspace/iptables.c	2003-05-02 14:20:59.000000000 +0200
@@ -1668,6 +1668,7 @@
 	char *protocol = NULL;
 	const char *modprobe = NULL;
 	int proto_used = 0;
+	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2148,8 +2149,10 @@
 			   chain, IPT_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle)
+	if (!*handle) {
 		*handle = iptc_init(*table);
+		no_handle = 1;
+	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2294,5 +2297,10 @@
 	if (verbose > 1)
 		dump_entries(*handle);
 
+	if (no_handle) {
+		iptc_exit(*handle);
+		*handle = NULL;
+	}
+
 	return ret;
 }
diff -x '*.d' -urN userspace.orig/libiptc/libip4tc.c userspace/libiptc/libip4tc.c
--- userspace.orig/libiptc/libip4tc.c	2002-06-12 21:22:29.000000000 +0200
+++ userspace/libiptc/libip4tc.c	2003-05-02 13:48:30.000000000 +0200
@@ -91,6 +91,7 @@
 #define TC_SET_POLICY		iptc_set_policy
 #define TC_GET_RAW_SOCKET	iptc_get_raw_socket
 #define TC_INIT			iptc_init
+#define TC_EXIT			iptc_exit
 #define TC_COMMIT		iptc_commit
 #define TC_STRERROR		iptc_strerror
 
diff -x '*.d' -urN userspace.orig/libiptc/libip6tc.c userspace/libiptc/libip6tc.c
--- userspace.orig/libiptc/libip6tc.c	2002-02-14 00:13:23.000000000 +0100
+++ userspace/libiptc/libip6tc.c	2003-05-02 13:48:48.000000000 +0200
@@ -86,6 +86,7 @@
 #define TC_SET_POLICY		ip6tc_set_policy
 #define TC_GET_RAW_SOCKET	ip6tc_get_raw_socket
 #define TC_INIT			ip6tc_init
+#define TC_EXIT			ip6tc_exit
 #define TC_COMMIT		ip6tc_commit
 #define TC_STRERROR		ip6tc_strerror
 
diff -x '*.d' -urN userspace.orig/libiptc/libiptc.c userspace/libiptc/libiptc.c
--- userspace.orig/libiptc/libiptc.c	2003-04-30 18:56:19.000000000 +0200
+++ userspace/libiptc/libiptc.c	2003-05-02 14:22:40.000000000 +0200
@@ -237,22 +237,26 @@
 	if (sockfd != -1)
 		close(sockfd);
 
+	if (strlen(tablename) >= TABLE_MAXNAMELEN) {
+		errno = EINVAL;
+		return NULL;
+	}
+	
 	sockfd = socket(TC_AF, SOCK_RAW, IPPROTO_RAW);
 	if (sockfd < 0)
 		return NULL;
 
 	s = sizeof(info);
-	if (strlen(tablename) >= TABLE_MAXNAMELEN) {
-		errno = EINVAL;
-		return NULL;
-	}
+
 	strcpy(info.name, tablename);
 	if (getsockopt(sockfd, TC_IPPROTO, SO_GET_INFO, &info, &s) < 0)
 		return NULL;
 
 	if ((h = alloc_handle(info.name, info.size, info.num_entries))
-	    == NULL)
+	    == NULL) {
+		close(sockfd);
 		return NULL;
+	}
 
 /* Too hard --RR */
 #if 0
@@ -284,6 +288,7 @@
 
 	if (getsockopt(sockfd, TC_IPPROTO, SO_GET_ENTRIES, &h->entries,
 		       &tmp) < 0) {
+		close(sockfd);
 		free(h);
 		return NULL;
 	}
@@ -292,6 +297,13 @@
 	return h;
 }
 
+void
+TC_EXIT(TC_HANDLE_T h)
+{
+	close(sockfd);
+	free(h);
+}
+
 static inline int
 print_match(const STRUCT_ENTRY_MATCH *m)
 {
@@ -504,10 +516,8 @@
 	(*handle)->cache_chain_iteration++;
 
 	if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
-	    == (*handle)->cache_num_chains) {
-		free((*handle)->cache_chain_heads);
+	    == (*handle)->cache_num_chains)
 		return NULL;
-	}
 
 	return (*handle)->cache_chain_iteration->name;
 }
@@ -1584,11 +1594,13 @@
 	STRUCT_REPLACE *repl;
 	STRUCT_COUNTERS_INFO *newcounters;
 	unsigned int i;
-	size_t counterlen
-		= sizeof(STRUCT_COUNTERS_INFO)
-		+ sizeof(STRUCT_COUNTERS) * (*handle)->new_number;
+	size_t counterlen;
 
 	CHECK(*handle);
+
+	counterlen = sizeof(STRUCT_COUNTERS_INFO)
+			+ sizeof(STRUCT_COUNTERS) * (*handle)->new_number;
+
 #if 0
 	TC_DUMP_ENTRIES(*handle);
 #endif

Re: libiptc memory leak - PATCH

[Martin Josefsson]

On Fri, 2003-05-02 at 14:47, Martin Josefsson wrote:

> This patch breaks {ip,ip6}tables-{save,restore} and it runs free() on
> the wrong memoryaddress (I think free() does the right thing anyway but
> the patch still breaks stuff).

Ok, I was wrong, it did free on a correct memoryaddress, I'll fix up my
patch and resend, sorry about that.

-- 
/Martin

Re: libiptc memory leak - PATCH

[Martin Josefsson]

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

On Fri, 2003-05-02 at 15:47, Martin Josefsson wrote:
> On Fri, 2003-05-02 at 14:47, Martin Josefsson wrote:
> 
> > This patch breaks {ip,ip6}tables-{save,restore} and it runs free() on
> > the wrong memoryaddress (I think free() does the right thing anyway but
> > the patch still breaks stuff).
> 
> Ok, I was wrong, it did free on a correct memoryaddress, I'll fix up my
> patch and resend, sorry about that.

Here's an updated patch that fixes the memory-leak in the previous one.

I've renamed {ip,ip6}tc_exit(iptc_handle_t h) into
{ip,ip6}tc_free(iptc_handle_t *h)

This should fix it correctly (I hope), iptables, iptables-{save,restore}
works fine.

-- 
/Martin

[-- Attachment #2: libiptc-diff2 --]
[-- Type: text/x-patch, Size: 7514 bytes --]

diff -x '*.d' -urN userspace.orig/include/libiptc/libiptc.h userspace/include/libiptc/libiptc.h
--- userspace.orig/include/libiptc/libiptc.h	2001-04-19 18:35:39.000000000 +0200
+++ userspace/include/libiptc/libiptc.h	2003-05-02 16:00:06.000000000 +0200
@@ -34,6 +34,9 @@
 /* Take a snapshot of the rules.  Returns NULL on error. */
 iptc_handle_t iptc_init(const char *tablename);
 
+/* Cleanup after iptc_init(). */
+void iptc_free(iptc_handle_t *h);
+
 /* Iterator functions to run through the chains.  Returns NULL at end. */
 const char *iptc_first_chain(iptc_handle_t *handle);
 const char *iptc_next_chain(iptc_handle_t *handle);
diff -x '*.d' -urN userspace.orig/ip6tables-restore.c userspace/ip6tables-restore.c
--- userspace.orig/ip6tables-restore.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/ip6tables-restore.c	2003-05-02 16:01:08.000000000 +0200
@@ -102,7 +102,7 @@
 
 int main(int argc, char *argv[])
 {
-	ip6tc_handle_t handle;
+	ip6tc_handle_t handle = NULL;
 	char buffer[10240];
 	int c;
 	char curtable[IP6T_TABLE_MAXNAMELEN + 1];
@@ -183,6 +183,9 @@
 			}
 			strncpy(curtable, table, IP6T_TABLE_MAXNAMELEN);
 
+			if (handle)
+				ip6tc_free(&handle);
+
 			handle = create_handle(table, modprobe);
 			if (noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
diff -x '*.d' -urN userspace.orig/ip6tables-save.c userspace/ip6tables-save.c
--- userspace.orig/ip6tables-save.c	2002-08-14 13:40:41.000000000 +0200
+++ userspace/ip6tables-save.c	2003-05-02 16:01:17.000000000 +0200
@@ -305,6 +305,8 @@
 		exit_error(OTHER_PROBLEM, "Binary NYI\n");
 	}
 
+	ip6tc_free(&h);
+
 	return 1;
 }
 
diff -x '*.d' -urN userspace.orig/ip6tables.c userspace/ip6tables.c
--- userspace.orig/ip6tables.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/ip6tables.c	2003-05-02 16:00:58.000000000 +0200
@@ -1670,6 +1670,7 @@
 	const char *modprobe = NULL;
 	int proto_used = 0;
 	char icmp6p[] = "icmpv6";
+	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2147,8 +2148,10 @@
 			   chain, IP6T_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle)
+	if (!*handle) {
 		*handle = ip6tc_init(*table);
+		no_handle = 1;
+	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2293,5 +2296,8 @@
 	if (verbose > 1)
 		dump_entries6(*handle);
 
+	if (no_handle)
+		ip6tc_free(handle);
+
 	return ret;
 }
diff -x '*.d' -urN userspace.orig/iptables-restore.c userspace/iptables-restore.c
--- userspace.orig/iptables-restore.c	2003-03-06 13:01:32.000000000 +0100
+++ userspace/iptables-restore.c	2003-05-02 16:00:34.000000000 +0200
@@ -99,7 +99,7 @@
 
 int main(int argc, char *argv[])
 {
-	iptc_handle_t handle;
+	iptc_handle_t handle = NULL;
 	char buffer[10240];
 	int c;
 	char curtable[IPT_TABLE_MAXNAMELEN + 1];
@@ -180,6 +180,9 @@
 			}
 			strncpy(curtable, table, IPT_TABLE_MAXNAMELEN);
 
+			if (handle)
+				iptc_free(&handle);
+
 			handle = create_handle(table, modprobe);
 			if (noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
diff -x '*.d' -urN userspace.orig/iptables-save.c userspace/iptables-save.c
--- userspace.orig/iptables-save.c	2002-08-07 11:07:41.000000000 +0200
+++ userspace/iptables-save.c	2003-05-02 16:00:23.000000000 +0200
@@ -304,6 +304,8 @@
 		exit_error(OTHER_PROBLEM, "Binary NYI\n");
 	}
 
+	iptc_free(&h);
+
 	return 1;
 }
 
diff -x '*.d' -urN userspace.orig/iptables.c userspace/iptables.c
--- userspace.orig/iptables.c	2003-03-31 14:33:55.000000000 +0200
+++ userspace/iptables.c	2003-05-02 16:00:46.000000000 +0200
@@ -1668,6 +1668,7 @@
 	char *protocol = NULL;
 	const char *modprobe = NULL;
 	int proto_used = 0;
+	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2148,8 +2149,10 @@
 			   chain, IPT_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle)
+	if (!*handle) {
 		*handle = iptc_init(*table);
+		no_handle = 1;
+	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2294,5 +2297,8 @@
 	if (verbose > 1)
 		dump_entries(*handle);
 
+	if (no_handle)
+		iptc_free(handle);
+
 	return ret;
 }
diff -x '*.d' -urN userspace.orig/libiptc/libip4tc.c userspace/libiptc/libip4tc.c
--- userspace.orig/libiptc/libip4tc.c	2002-06-12 21:22:29.000000000 +0200
+++ userspace/libiptc/libip4tc.c	2003-05-02 15:59:39.000000000 +0200
@@ -91,6 +91,7 @@
 #define TC_SET_POLICY		iptc_set_policy
 #define TC_GET_RAW_SOCKET	iptc_get_raw_socket
 #define TC_INIT			iptc_init
+#define TC_FREE			iptc_free
 #define TC_COMMIT		iptc_commit
 #define TC_STRERROR		iptc_strerror
 
diff -x '*.d' -urN userspace.orig/libiptc/libip6tc.c userspace/libiptc/libip6tc.c
--- userspace.orig/libiptc/libip6tc.c	2002-02-14 00:13:23.000000000 +0100
+++ userspace/libiptc/libip6tc.c	2003-05-02 15:59:49.000000000 +0200
@@ -86,6 +86,7 @@
 #define TC_SET_POLICY		ip6tc_set_policy
 #define TC_GET_RAW_SOCKET	ip6tc_get_raw_socket
 #define TC_INIT			ip6tc_init
+#define TC_FREE			ip6tc_free
 #define TC_COMMIT		ip6tc_commit
 #define TC_STRERROR		ip6tc_strerror
 
diff -x '*.d' -urN userspace.orig/libiptc/libiptc.c userspace/libiptc/libiptc.c
--- userspace.orig/libiptc/libiptc.c	2003-04-30 18:56:19.000000000 +0200
+++ userspace/libiptc/libiptc.c	2003-05-02 16:03:47.000000000 +0200
@@ -237,22 +237,26 @@
 	if (sockfd != -1)
 		close(sockfd);
 
+	if (strlen(tablename) >= TABLE_MAXNAMELEN) {
+		errno = EINVAL;
+		return NULL;
+	}
+	
 	sockfd = socket(TC_AF, SOCK_RAW, IPPROTO_RAW);
 	if (sockfd < 0)
 		return NULL;
 
 	s = sizeof(info);
-	if (strlen(tablename) >= TABLE_MAXNAMELEN) {
-		errno = EINVAL;
-		return NULL;
-	}
+
 	strcpy(info.name, tablename);
 	if (getsockopt(sockfd, TC_IPPROTO, SO_GET_INFO, &info, &s) < 0)
 		return NULL;
 
 	if ((h = alloc_handle(info.name, info.size, info.num_entries))
-	    == NULL)
+	    == NULL) {
+		close(sockfd);
 		return NULL;
+	}
 
 /* Too hard --RR */
 #if 0
@@ -284,6 +288,7 @@
 
 	if (getsockopt(sockfd, TC_IPPROTO, SO_GET_ENTRIES, &h->entries,
 		       &tmp) < 0) {
+		close(sockfd);
 		free(h);
 		return NULL;
 	}
@@ -292,6 +297,16 @@
 	return h;
 }
 
+void
+TC_FREE(TC_HANDLE_T *h)
+{
+	close(sockfd);
+	if ((*h)->cache_chain_heads)
+		free((*h)->cache_chain_heads);
+	free(*h);
+	*h = NULL;
+}
+
 static inline int
 print_match(const STRUCT_ENTRY_MATCH *m)
 {
@@ -504,10 +519,8 @@
 	(*handle)->cache_chain_iteration++;
 
 	if ((*handle)->cache_chain_iteration - (*handle)->cache_chain_heads
-	    == (*handle)->cache_num_chains) {
-		free((*handle)->cache_chain_heads);
+	    == (*handle)->cache_num_chains)
 		return NULL;
-	}
 
 	return (*handle)->cache_chain_iteration->name;
 }
@@ -1584,11 +1597,13 @@
 	STRUCT_REPLACE *repl;
 	STRUCT_COUNTERS_INFO *newcounters;
 	unsigned int i;
-	size_t counterlen
-		= sizeof(STRUCT_COUNTERS_INFO)
-		+ sizeof(STRUCT_COUNTERS) * (*handle)->new_number;
+	size_t counterlen;
 
 	CHECK(*handle);
+
+	counterlen = sizeof(STRUCT_COUNTERS_INFO)
+			+ sizeof(STRUCT_COUNTERS) * (*handle)->new_number;
+
 #if 0
 	TC_DUMP_ENTRIES(*handle);
 #endif
@@ -1715,10 +1730,7 @@
 	free(newcounters);
 
  finished:
-	if ((*handle)->cache_chain_heads)
-		free((*handle)->cache_chain_heads);
-	free(*handle);
-	*handle = NULL;
+	TC_FREE(handle);
 	return 1;
 }
 

Re: libiptc memory leak - PATCH

[Harald Welte]

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

On Fri, May 02, 2003 at 02:47:51PM +0200, Martin Josefsson wrote:
> On Wed, 2003-04-30 at 17:47, Harald Welte wrote:
> > On Mon, Apr 28, 2003 at 09:14:25AM +0200, Tomá? Lejdar wrote:
> > > Hi,
> > > I thing there is no problem, the memory is freed only when the
> > > function next_chain returns NULL and run through the chains is on the end.
> > > In the next call of first_chain() is memory alocated again in 
> > > populate_cache().
> > 
> > no, it's fine.  It was a misunderstanding. I'll apply your patch now.
> > Sorry for the fuzz.
> 
> This patch breaks {ip,ip6}tables-{save,restore} and it runs free() on
> the wrong memoryaddress (I think free() does the right thing anyway but
> the patch still breaks stuff).

*sigh*.  I knew that I should have tested it before accepting.

> I've attached a patch that reverts this patch and adds
> iptc_exit(iptc_handle_t h) that takes care of the freeing.
> It also contains some small cleanups.

I'll apply your patch, thanks Martin.

> /Martin

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: libiptc memory leak - PATCH

[Harald Welte]

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

On Fri, May 02, 2003 at 04:14:19PM +0200, Martin Josefsson wrote:
> On Fri, 2003-05-02 at 15:47, Martin Josefsson wrote:
> > On Fri, 2003-05-02 at 14:47, Martin Josefsson wrote:
> > 
> > > This patch breaks {ip,ip6}tables-{save,restore} and it runs free() on
> > > the wrong memoryaddress (I think free() does the right thing anyway but
> > > the patch still breaks stuff).
> > 
> > Ok, I was wrong, it did free on a correct memoryaddress, I'll fix up my
> > patch and resend, sorry about that.
> 
> Here's an updated patch that fixes the memory-leak in the previous one.

applied this one instead of the other.

> This should fix it correctly (I hope), iptables, iptables-{save,restore}
> works fine.

we'll see.  I think iptables-1.2.9 is in a quite distant future, so
I hope we discover any potentially introduced bugs until then.

> /Martin

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: libiptc memory leak - PATCH

[Martin Josefsson]

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

On Fri, 2003-05-02 at 17:21, Harald Welte wrote:

> > This should fix it correctly (I hope), iptables, iptables-{save,restore}
> > works fine.
> 
> we'll see.  I think iptables-1.2.9 is in a quite distant future, so
> I hope we discover any potentially introduced bugs until then.

I broke it a little...

I've attached a fix...
it fixes iptables.c and ip6tables.c

-- 
/Martin

[-- Attachment #2: iptables.c-fix --]
[-- Type: text/plain, Size: 1471 bytes --]

--- userspace/iptables.c.orig	2003-05-02 17:31:44.000000000 +0200
+++ userspace/iptables.c	2003-05-03 22:43:39.000000000 +0200
@@ -1668,7 +1668,6 @@
 	char *protocol = NULL;
 	const char *modprobe = NULL;
 	int proto_used = 0;
-	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2149,10 +2148,8 @@
 			   chain, IPT_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle) {
+	if (!*handle)
 		*handle = iptc_init(*table);
-		no_handle = 1;
-	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2297,8 +2294,5 @@
 	if (verbose > 1)
 		dump_entries(*handle);
 
-	if (no_handle)
-		iptc_free(handle);
-
 	return ret;
 }
--- userspace/ip6tables.c.orig	2003-05-02 17:31:44.000000000 +0200
+++ userspace/ip6tables.c	2003-05-03 22:50:51.000000000 +0200
@@ -1670,7 +1670,6 @@
 	const char *modprobe = NULL;
 	int proto_used = 0;
 	char icmp6p[] = "icmpv6";
-	int no_handle = 0;
 
 	memset(&fw, 0, sizeof(fw));
 
@@ -2148,10 +2147,8 @@
 			   chain, IP6T_FUNCTION_MAXNAMELEN);
 
 	/* only allocate handle if we weren't called with a handle */
-	if (!*handle) {
+	if (!*handle)
 		*handle = ip6tc_init(*table);
-		no_handle = 1;
-	}
 
 	if (!*handle) {
 		/* try to insmod the module if iptc_init failed */
@@ -2296,8 +2293,5 @@
 	if (verbose > 1)
 		dump_entries6(*handle);
 
-	if (no_handle)
-		ip6tc_free(handle);
-
 	return ret;
 }