[PATCH 0/8] Possible problems found by static analysis of code

[PATCH 0/8] Possible problems found by static analysis of code

[Jiri Popelka]

We had analyzed the iptables-1.4.10 code with Coverity.
Coverity is commercial enterprise level tool for
static analysis (analysis based only on compiling of sources,
not based on running of binary) of the code.

As a result I have the following patches that should fix some possible problems.
There's a respective part(s) of the Coverity error log in each commit comment.

You could also find this link useful:
https://www.securecoding.cert.org/confluence/display/seccode/Coverity+Prevent

Jiri Popelka (8):
  iptables: Coverity: DEADCODE
  iptables: Coverity: FORWARD_NULL
  iptables: Coverity: NEGATIVE_RETURNS
  iptables: Coverity: REVERSE_INULL
  iptables: Coverity: UNINIT
  iptables: Coverity: VARARGS
  iptables: Coverity: OVERRUN_STATIC
  iptables: Coverity: RESOURCE_LEAK

 extensions/libip6t_REJECT.c  |   13 +++++++------
 extensions/libipt_REJECT.c   |   11 ++++++-----
 extensions/libxt_multiport.c |    2 --
 extensions/libxt_sctp.c      |    2 +-
 iptables/ip6tables-restore.c |    3 +--
 iptables/ip6tables.c         |    5 ++++-
 iptables/iptables-restore.c  |    5 ++---
 iptables/iptables-xml.c      |    6 +++---
 iptables/iptables.c          |    8 ++++++--
 iptables/xtables.c           |   15 +++++++++++----
 libipq/libipq.c              |    1 -
 libiptc/libiptc.c            |    8 +++-----
 12 files changed, 44 insertions(+), 35 deletions(-)

-- 
1.7.5.2

[PATCH 1/8] iptables: Coverity: DEADCODE

[Jiri Popelka]

libiptc/libiptc.c:407: dead_error_condition: On this path, the condition "res > 0" cannot be false.
libiptc/libiptc.c:396: at_least: After this line, the value of "res" is at least 1.
libiptc/libiptc.c:393: equality_cond: Condition "res == 0" is evaluated as false.
libiptc/libiptc.c:396: new_values: Noticing condition "res < 0".
libiptc/libiptc.c:425: new_values: Noticing condition "res < 0".
libiptc/libiptc.c:407: new_values: Noticing condition "res > 0".
libiptc/libiptc.c:435: dead_error_line: Execution cannot reach this statement "return list_pos;".
---
 libiptc/libiptc.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 0b6d5e3..c2cb0bc 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -403,7 +403,7 @@ __iptcc_bsearch_chain_index(const char *name, unsigned int offset,
 		}
 		debug("jump back to pos:%d (end:%d)\n", pos, end);
 		goto loop;
-	} else if (res > 0 ){ /* Not far enough, jump forward */
+	} else { /* Not far enough, jump forward */
 
 		/* Exit case: Last element of array */
 		if (pos == handle->chain_index_sz-1) {
@@ -430,8 +430,6 @@ __iptcc_bsearch_chain_index(const char *name, unsigned int offset,
 		debug("jump forward to pos:%d (end:%d)\n", pos, end);
 		goto loop;
 	}
-
-	return list_pos;
 }
 
 /* Wrapper for string chain name based bsearch */
-- 
1.7.5.2

[PATCH 2/8] iptables: Coverity: FORWARD_NULL

[Jiri Popelka]

ip6tables.c:1841: var_compare_op: Comparing "chain" to null implies that "chain" might be null.
ip6tables.c:1863: var_deref_model: Passing null variable "chain" to function "strcmp", which
                  dereferences it. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
ip6tables.c:1946: var_deref_model: Passing null variable "chain" to function "ip6tc_delete_num_entry", which dereferences it.
libiptc/libiptc.c:2050: deref_parm_in_call: Function "iptcc_find_label" dereferences parameter "chain".
libiptc/libiptc.c:737: deref_parm_in_call: Function "strcmp" dereferences parameter "name".
                       (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
ip6tables.c:1967: var_deref_model: Passing null variable "chain" to function "ip6tc_zero_counter", which dereferences it.
ip6tables.c:1999: var_deref_model: Passing null variable "chain" to function "ip6tc_create_chain", which dereferences it.
ip6tables.c:2005: var_deref_model: Passing null variable "chain" to function "ip6tc_rename_chain", which dereferences it.
ip6tables.c:2008: var_deref_model: Passing null variable "chain" to function "ip6tc_set_policy", which dereferences it.

iptables.c:1879: var_compare_op: Comparing "chain" to null implies that "chain" might be null.
iptables.c:1901: var_deref_model: Passing null variable "chain" to function "strcmp", which
                 dereferences it. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
iptables.c:1986: var_deref_model: Passing null variable "chain" to function "iptc_delete_num_entry", which dereferences it.
libiptc/libiptc.c:2050: deref_parm_in_call: Function "iptcc_find_label" dereferences parameter "chain".
libiptc/libiptc.c:737: deref_parm_in_call: Function "strcmp" dereferences parameter "name".
                       (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
iptables.c:2007: var_deref_model: Passing null variable "chain" to function "iptc_zero_counter", which dereferences it.
iptables.c:2039: var_deref_model: Passing null variable "chain" to function "iptc_create_chain", which dereferences it.
iptables.c:2045: var_deref_model: Passing null variable "chain" to function "iptc_rename_chain", which dereferences it.
iptables.c:2048: var_deref_model: Passing null variable "chain" to function "iptc_set_policy", which dereferences it.

iptables.c:1828: var_compare_op: Comparing "policy" to null implies that "policy" might be null.
iptables.c:2048: var_deref_model: Passing null variable "policy" to function "iptc_set_policy", which dereferences it.
libiptc/libiptc.c:2422: deref_parm_in_call: Function "strcmp" dereferences parameter "policy".
                        (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)

iptables-xml.c:745: assign_zero: Assigning: "chain" = 0.
iptables-xml.c:855: var_deref_model: Passing null variable "chain" to function "needChain", which dereferences it.
iptables-xml.c:282: deref_parm_in_call: Function "strcmp" dereferences parameter "chain".
                    (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)
---
 iptables/ip6tables.c    |    5 ++++-
 iptables/iptables-xml.c |    3 ++-
 iptables/iptables.c     |    8 ++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 4037acf..b30c9b7 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1770,7 +1770,10 @@ int do_command6(int argc, char *argv[], char **table, struct ip6tc_handle **hand
 
 	generic_opt_check(command, cs.options);
 
-	if (chain != NULL && strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
+	if (chain == NULL)
+		xtables_error(PARAMETER_PROBLEM, "no chain");
+
+	if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
 			   "chain name `%s' too long (must be under %u chars)",
 			   chain, XT_EXTENSION_MAXNAMELEN);
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 5aa638c..e2cb809 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -847,7 +847,8 @@ main(int argc, char *argv[])
 			for (a = 0; a < newargc; a++)
 				DEBUGP("argv[%u]: %s\n", a, newargv[a]);
 
-			needChain(chain);// Should we explicitly look for -A
+			if (chain != NULL)
+				needChain(chain);// Should we explicitly look for -A
 			do_rule(pcnt, bcnt, newargc, newargv, newargvattr);
 
 			save_argv();
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 4ae7541..4868e40 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1800,7 +1800,10 @@ int do_command4(int argc, char *argv[], char **table, struct iptc_handle **handl
 
 	generic_opt_check(command, cs.options);
 
-	if (chain != NULL && strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
+	if (chain == NULL)
+		xtables_error(PARAMETER_PROBLEM, "no chain");
+
+	if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
 			   "chain name `%s' too long (must be under %u chars)",
 			   chain, XT_EXTENSION_MAXNAMELEN);
@@ -1978,7 +1981,8 @@ int do_command4(int argc, char *argv[], char **table, struct iptc_handle **handl
 		ret = iptc_rename_chain(chain, newname,	*handle);
 		break;
 	case CMD_SET_POLICY:
-		ret = iptc_set_policy(chain, policy, cs.options&OPT_COUNTERS ? &cs.fw.counters : NULL, *handle);
+		if (policy != NULL)
+			ret = iptc_set_policy(chain, policy, cs.options&OPT_COUNTERS ? &cs.fw.counters : NULL, *handle);
 		break;
 	default:
 		/* We should never reach this... */
-- 
1.7.5.2

[PATCH 3/8] iptables: Coverity: NEGATIVE_RETURNS

[Jiri Popelka]

libipq/libipq.c:232: var_tested_neg: Variable "h->fd" tests negative.
libipq/libipq.c:234: negative_returns: "h->fd" is passed to a parameter that cannot be negative.
---
 libipq/libipq.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/libipq/libipq.c b/libipq/libipq.c
index e330487..fb65971 100644
--- a/libipq/libipq.c
+++ b/libipq/libipq.c
@@ -231,7 +231,6 @@ struct ipq_handle *ipq_create_handle(uint32_t flags, uint32_t protocol)
         
 	if (h->fd == -1) {
 		ipq_errno = IPQ_ERR_SOCKET;
-		close(h->fd);
 		free(h);
 		return NULL;
 	}
-- 
1.7.5.2

[PATCH 4/8] iptables: Coverity: REVERSE_INULL

[Jiri Popelka]

ip6tables-restore.c:186: deref_ptr_in_call: Dereferencing pointer "in".
ip6tables-restore.c:463: check_after_deref: Dereferencing "in" before a null check.

iptables-restore.c:192: deref_ptr_in_call: Dereferencing pointer "in".
iptables-restore.c:468: check_after_deref: Dereferencing "in" before a null check.

iptables-xml.c:671: deref_ptr_in_call: Dereferencing pointer "in".
iptables-xml.c:873: check_after_deref: Dereferencing "in" before a null check.
---
 iptables/ip6tables-restore.c |    3 +--
 iptables/iptables-restore.c  |    3 +--
 iptables/iptables-xml.c      |    3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 420bc52..ae147d5 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -460,7 +460,6 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (in != NULL)
-		fclose(in);
+	fclose(in);
 	return 0;
 }
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 2624599..1cb833c 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -465,7 +465,6 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (in != NULL)
-		fclose(in);
+	fclose(in);
 	return 0;
 }
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index e2cb809..f340889 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -866,8 +866,7 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (in != NULL)
-		fclose(in);
+	fclose(in);
 	printf("</iptables-rules>\n");
 	free_argv();
 
-- 
1.7.5.2

[PATCH 5/8] iptables: Coverity: UNINIT

[Jiri Popelka]

extensions/libxt_multiport.c:102: var_decl: Declaring variable "m" without initializer.
extensions/libxt_multiport.c:127: uninit_use: Using uninitialized value "m".

xtables.c:1496: var_decl: Declaring variable "n" without initializer.
xtables.c:1568: uninit_use: Using uninitialized value "n".

iptables-restore.c:128: var_decl: Declaring variable "curtable" without initializer.
iptables-restore.c:454: uninit_use_in_call: Using uninitialized element
                        of array "curtable" when calling "strcmp".
---
 extensions/libxt_multiport.c |    2 --
 iptables/iptables-restore.c  |    2 +-
 iptables/xtables.c           |    2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/extensions/libxt_multiport.c b/extensions/libxt_multiport.c
index 03af5a9..63f96d9 100644
--- a/extensions/libxt_multiport.c
+++ b/extensions/libxt_multiport.c
@@ -108,7 +108,6 @@ parse_multi_ports_v1(const char *portstring,
 {
 	char *buffer, *cp, *next, *range;
 	unsigned int i;
-	uint16_t m;
 
 	buffer = strdup(portstring);
 	if (!buffer) xtables_error(OTHER_PROBLEM, "strdup failed");
@@ -133,7 +132,6 @@ parse_multi_ports_v1(const char *portstring,
 			if (multiinfo->ports[i-1] >= multiinfo->ports[i])
 				xtables_error(PARAMETER_PROBLEM,
 					   "invalid portrange specified");
-			m <<= 1;
 		}
  	}
 	multiinfo->count = i;
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 1cb833c..5df0ef2 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -451,7 +451,7 @@ main(int argc, char *argv[])
 			free_argv();
 			fflush(stdout);
 		}
-		if (tablename && (strcmp(tablename, curtable) != 0))
+		if (tablename && curtable && (strcmp(tablename, curtable) != 0))
 			continue;
 		if (!ret) {
 			fprintf(stderr, "%s: line %u failed\n",
diff --git a/iptables/xtables.c b/iptables/xtables.c
index acfcf8b..ee38421 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1605,7 +1605,7 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
 	static const struct in6_addr zero_addr;
 	struct in6_addr *addrp;
 	char buf[256], *p;
-	unsigned int len, i, j, n, count = 1;
+	unsigned int len, i, j, n = 0, count = 1;
 	const char *loop = name;
 
 	while ((loop = strchr(loop, ',')) != NULL) {
-- 
1.7.5.2

[PATCH 6/8] iptables: Coverity: VARARGS

[Jiri Popelka]

xtables.c:931: va_init: Initializing va_list "args".
xtables.c:938: missing_va_end: va_end was not called for "args".
xtables.c:947: missing_va_end: va_end was not called for "args".
xtables.c:961: missing_va_end: va_end was not called for "args".
---
 iptables/xtables.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/iptables/xtables.c b/iptables/xtables.c
index ee38421..9a43612 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1042,8 +1042,10 @@ void xtables_param_act(unsigned int status, const char *p1, ...)
 	case XTF_ONLY_ONCE:
 		p2 = va_arg(args, const char *);
 		b  = va_arg(args, unsigned int);
-		if (!b)
+		if (!b) {
+			va_end(args);
 			return;
+		}
 		xt_params->exit_err(PARAMETER_PROBLEM,
 		           "%s: \"%s\" option may only be specified once",
 		           p1, p2);
@@ -1051,8 +1053,10 @@ void xtables_param_act(unsigned int status, const char *p1, ...)
 	case XTF_NO_INVERT:
 		p2 = va_arg(args, const char *);
 		b  = va_arg(args, unsigned int);
-		if (!b)
+		if (!b) {
+			va_end(args);
 			return;
+		}
 		xt_params->exit_err(PARAMETER_PROBLEM,
 		           "%s: \"%s\" option cannot be inverted", p1, p2);
 		break;
@@ -1065,8 +1069,10 @@ void xtables_param_act(unsigned int status, const char *p1, ...)
 		break;
 	case XTF_ONE_ACTION:
 		b = va_arg(args, unsigned int);
-		if (!b)
+		if (!b) {
+			va_end(args);
 			return;
+		}
 		xt_params->exit_err(PARAMETER_PROBLEM,
 		           "%s: At most one action is possible", p1);
 		break;
-- 
1.7.5.2

[PATCH 7/8] iptables: Coverity: OVERRUN_STATIC

[Jiri Popelka]

extensions/libip6t_REJECT.c:114: overrun-local: Overrunning static array "reject_table",
                                 with 5 elements, at position 5 with index variable "i".

extensions/libip6t_REJECT.c:127: overrun-local: Overrunning static array "reject_table",
                                 with 5 elements, at position 5 with index variable "i".

extensions/libipt_REJECT.c:135: overrun-local: Overrunning static array "reject_table",
                                with 8 elements, at position 8 with index variable "i".

extensions/libipt_REJECT.c:148: overrun-local: Overrunning static array "reject_table",
                                with 8 elements, at position 8 with index variable "i".

extensions/libxt_sctp.c:419: overrun-call: Overrunning callee's array of size 18 by passing
                             index "i" of value 255 in call to function "print_chunk(i, numeric)".
extensions/libxt_sctp.c:383: index_parm: Directly indexing parameter.

libiptc/libiptc.c:1690: overrun-buffer-arg: Overrunning static array "t->target.u.user.name"
                        of size 29 bytes by passing it to a function which indexes it
                        with argument "30UL" at byte position 29.

libiptc/libiptc.c:1127: overrun-buffer-arg: Overrunning static array "t->target.u.user.name"
                        of size 29 bytes by passing it to a function which indexes it
                        with argument "30UL" at byte position 29.
---
 extensions/libip6t_REJECT.c |   13 +++++++------
 extensions/libipt_REJECT.c  |   11 ++++++-----
 extensions/libxt_sctp.c     |    2 +-
 libiptc/libiptc.c           |    4 ++--
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/extensions/libip6t_REJECT.c b/extensions/libip6t_REJECT.c
index 8085321..aeba996 100644
--- a/extensions/libip6t_REJECT.c
+++ b/extensions/libip6t_REJECT.c
@@ -27,7 +27,7 @@ static const struct reject_names reject_table[] = {
 	{"icmp6-adm-prohibited", "adm-prohibited",
 		IP6T_ICMP6_ADM_PROHIBITED, "ICMPv6 administratively prohibited"},
 #if 0
-	{"icmp6-not-neighbor", "not-neighbor"},
+	{"icmp6-not-neighbor", "not-neighbor",
 		IP6T_ICMP6_NOT_NEIGHBOR, "ICMPv6 not a neighbor"},
 #endif
 	{"icmp6-addr-unreachable", "addr-unreach",
@@ -102,9 +102,10 @@ static void REJECT_print(const void *ip, const struct xt_entry_target *target,
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(reject_table); ++i)
-		if (reject_table[i].with == reject->with)
+		if (reject_table[i].with == reject->with) {
+			printf(" reject-with %s", reject_table[i].name);
 			break;
-	printf(" reject-with %s", reject_table[i].name);
+		}
 }
 
 static void REJECT_save(const void *ip, const struct xt_entry_target *target)
@@ -114,10 +115,10 @@ static void REJECT_save(const void *ip, const struct xt_entry_target *target)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(reject_table); ++i)
-		if (reject_table[i].with == reject->with)
+		if (reject_table[i].with == reject->with) {
+			printf(" --reject-with %s", reject_table[i].name);
 			break;
-
-	printf(" --reject-with %s", reject_table[i].name);
+		}
 }
 
 static struct xtables_target reject_tg6_reg = {
diff --git a/extensions/libipt_REJECT.c b/extensions/libipt_REJECT.c
index 362c65e..39ddeb5 100644
--- a/extensions/libipt_REJECT.c
+++ b/extensions/libipt_REJECT.c
@@ -122,9 +122,10 @@ static void REJECT_print(const void *ip, const struct xt_entry_target *target,
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(reject_table); ++i)
-		if (reject_table[i].with == reject->with)
+		if (reject_table[i].with == reject->with) {
+			printf(" reject-with %s", reject_table[i].name);
 			break;
-	printf(" reject-with %s", reject_table[i].name);
+		}
 }
 
 static void REJECT_save(const void *ip, const struct xt_entry_target *target)
@@ -134,10 +135,10 @@ static void REJECT_save(const void *ip, const struct xt_entry_target *target)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(reject_table); ++i)
-		if (reject_table[i].with == reject->with)
+		if (reject_table[i].with == reject->with) {
+			printf(" --reject-with %s", reject_table[i].name);
 			break;
-
-	printf(" --reject-with %s", reject_table[i].name);
+		}
 }
 
 static struct xtables_target reject_tg_reg = {
diff --git a/extensions/libxt_sctp.c b/extensions/libxt_sctp.c
index 5dbc36f..da9fb22 100644
--- a/extensions/libxt_sctp.c
+++ b/extensions/libxt_sctp.c
@@ -374,7 +374,7 @@ print_chunk(uint32_t chunknum, int numeric)
 
 		for (i = 0; i < ARRAY_SIZE(sctp_chunk_names); ++i)
 			if (sctp_chunk_names[i].chunk_type == chunknum)
-				printf("%s", sctp_chunk_names[chunknum].name);
+				printf("%s", sctp_chunk_names[i].name);
 	}
 }
 
diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index c2cb0bc..1a99047 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1121,7 +1121,7 @@ static inline int iptcc_compile_rule (struct xtc_handle *h, STRUCT_REPLACE *repl
 		STRUCT_STANDARD_TARGET *t;
 		t = (STRUCT_STANDARD_TARGET *)GET_TARGET(r->entry);
 		/* memset for memcmp convenience on delete/replace */
-		memset(t->target.u.user.name, 0, FUNCTION_MAXNAMELEN);
+		memset(t->target.u.user.name, 0, XT_EXTENSION_MAXNAMELEN);
 		strcpy(t->target.u.user.name, STANDARD_TARGET);
 		/* Jumps can only happen to builtin chains, so we
 		 * can safely assume that they always have a header */
@@ -1675,7 +1675,7 @@ iptcc_standard_map(struct rule_head *r, int verdict)
 		return 0;
 	}
 	/* memset for memcmp convenience on delete/replace */
-	memset(t->target.u.user.name, 0, FUNCTION_MAXNAMELEN);
+	memset(t->target.u.user.name, 0, XT_EXTENSION_MAXNAMELEN);
 	strcpy(t->target.u.user.name, STANDARD_TARGET);
 	t->verdict = verdict;
 
-- 
1.7.5.2

[PATCH 8/8] iptables: Coverity: RESOURCE_LEAK

[Jiri Popelka]

xtables.c:320: alloc_fn: Calling allocation function "get_modprobe".
xtables.c:294: alloc_fn: Storage is returned from allocation function "malloc".
xtables.c:294: var_assign: Assigning: "ret" = "malloc(1024UL)".
xtables.c:304: return_alloc: Returning allocated memory "ret".
xtables.c:320: var_assign: Assigning: "buf" =  storage returned from "get_modprobe()".
xtables.c:323: var_assign: Assigning: "modprobe" = "buf".
xtables.c:348: leaked_storage: Variable "buf" going out of scope
               leaks the storage it points to.
xtables.c:348: leaked_storage: Returning without freeing "modprobe"
               leaks the storage that it points to.
---
 iptables/xtables.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/iptables/xtables.c b/iptables/xtables.c
index 9a43612..951fca1 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -362,6 +362,7 @@ int xtables_insmod(const char *modname, const char *modprobe, bool quiet)
 		/* not usually reached */
 		exit(1);
 	case -1:
+		free(buf);
 		return -1;
 
 	default: /* parent */
-- 
1.7.5.2

Re: [PATCH 0/8] Possible problems found by static analysis of code

[Pablo Neira Ayuso]

On 10/06/11 15:25, Jiri Popelka wrote:
> We had analyzed the iptables-1.4.10 code with Coverity.
> Coverity is commercial enterprise level tool for
> static analysis (analysis based only on compiling of sources,
> not based on running of binary) of the code.

it would be cool if you pass that tool to the conntrack-tools and ipset.

Re: [PATCH 7/8] iptables: Coverity: OVERRUN_STATIC

[Jan Engelhardt]

On Friday 2011-06-10 15:26, Jiri Popelka wrote:
>
>libiptc/libiptc.c:1690: overrun-buffer-arg: Overrunning static array "t->target.u.user.name"
>                        of size 29 bytes by passing it to a function which indexes it
>                        with argument "30UL" at byte position 29.
>
>libiptc/libiptc.c:1127: overrun-buffer-arg: Overrunning static array "t->target.u.user.name"
>                        of size 29 bytes by passing it to a function which indexes it
>                        with argument "30UL" at byte position 29.

>diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
>index c2cb0bc..1a99047 100644
>--- a/libiptc/libiptc.c
>+++ b/libiptc/libiptc.c
>@@ -1121,7 +1121,7 @@ static inline int iptcc_compile_rule (struct xtc_handle *h, STRUCT_REPLACE *repl
> 		STRUCT_STANDARD_TARGET *t;
> 		t = (STRUCT_STANDARD_TARGET *)GET_TARGET(r->entry);
> 		/* memset for memcmp convenience on delete/replace */
>-		memset(t->target.u.user.name, 0, FUNCTION_MAXNAMELEN);
>+		memset(t->target.u.user.name, 0, XT_EXTENSION_MAXNAMELEN);
> 		strcpy(t->target.u.user.name, STANDARD_TARGET);
> 		/* Jumps can only happen to builtin chains, so we
> 		 * can safely assume that they always have a header */

I have a feeling that there was something... namely implicitly
setting t->target.u.user.revision too.

So this might need  a +t->target.u.user.revision = 0;
unless you want t->target.u.user.revision to contain.. something,
probably undefined value.

Re: [PATCH 0/8] Possible problems found by static analysis of code

[Jozsef Kadlecsik]

On Fri, 10 Jun 2011, Pablo Neira Ayuso wrote:

> On 10/06/11 15:25, Jiri Popelka wrote:
> > We had analyzed the iptables-1.4.10 code with Coverity.
> > Coverity is commercial enterprise level tool for
> > static analysis (analysis based only on compiling of sources,
> > not based on running of binary) of the code.
> 
> it would be cool if you pass that tool to the conntrack-tools and ipset.

Yes, I second that.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH 1/8] iptables: Coverity: DEADCODE

[Jan Engelhardt]

On Friday 2011-06-10 15:25, Jiri Popelka wrote:

>libiptc/libiptc.c:407: dead_error_condition: On this path, the condition "res > 0" cannot be false.
>libiptc/libiptc.c:396: at_least: After this line, the value of "res" is at least 1.
>libiptc/libiptc.c:393: equality_cond: Condition "res == 0" is evaluated as false.
>libiptc/libiptc.c:396: new_values: Noticing condition "res < 0".
>libiptc/libiptc.c:425: new_values: Noticing condition "res < 0".
>libiptc/libiptc.c:407: new_values: Noticing condition "res > 0".
>libiptc/libiptc.c:435: dead_error_line: Execution cannot reach this statement "return list_pos;".

Picked up by me.

Re: [PATCH 3/8] iptables: Coverity: NEGATIVE_RETURNS

[Jan Engelhardt]

On Friday 2011-06-10 15:25, Jiri Popelka wrote:

>libipq/libipq.c:232: var_tested_neg: Variable "h->fd" tests negative.
>libipq/libipq.c:234: negative_returns: "h->fd" is passed to a parameter that cannot be negative.

Picked up.

Re: [PATCH 4/8] iptables: Coverity: REVERSE_INULL

[Jan Engelhardt]

On Friday 2011-06-10 15:25, Jiri Popelka wrote:

>ip6tables-restore.c:186: deref_ptr_in_call: Dereferencing pointer "in".
>ip6tables-restore.c:463: check_after_deref: Dereferencing "in" before a null check.

Verified/Taken.

Re: [PATCH 6/8] iptables: Coverity: VARARGS

[Jan Engelhardt]

On Friday 2011-06-10 15:26, Jiri Popelka wrote:

>xtables.c:931: va_init: Initializing va_list "args".
>xtables.c:938: missing_va_end: va_end was not called for "args".
>xtables.c:947: missing_va_end: va_end was not called for "args".
>xtables.c:961: missing_va_end: va_end was not called for "args".

Taken.

Re: [PATCH 8/8] iptables: Coverity: RESOURCE_LEAK

[Jan Engelhardt]

On Friday 2011-06-10 15:26, Jiri Popelka wrote:

>xtables.c:320: alloc_fn: Calling allocation function "get_modprobe".
>xtables.c:294: alloc_fn: Storage is returned from allocation function "malloc".
>xtables.c:294: var_assign: Assigning: "ret" = "malloc(1024UL)".

Taken.