[PATCH 1/2 conntrack-tools] conntrackd: cthelper: Free pktb after use

[PATCH 1/2 conntrack-tools] conntrackd: cthelper: Free pktb after use

[Kevin Cernekee]

According to valgrind, this currently leaks ~512B to 2kB for each
packet sent to the userspace helper.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 src/cthelper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cthelper.c b/src/cthelper.c
index 54eb830..f01c509 100644
--- a/src/cthelper.c
+++ b/src/cthelper.c
@@ -325,6 +325,7 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
 	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
 		goto err4;
 
+	pktb_free(pktb);
 	nfct_destroy(ct);
 	if (myct->exp != NULL)
 		nfexp_destroy(myct->exp);
-- 
2.7.4

[PATCH 2/2 conntrack-tools] conntrackd: config: Free strdup()ed tokens

[Kevin Cernekee]

This frees T_IP, T_PATH_VAL, and T_STRING tokens.  They were being flagged
by valgrind as memory leaks.

Lightly tested using doc/helper/conntrackd.conf and doc/stats/conntrackd.conf.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 src/read_config_yy.y | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/src/read_config_yy.y b/src/read_config_yy.y
index 97f905d..f10e0db 100644
--- a/src/read_config_yy.y
+++ b/src/read_config_yy.y
@@ -116,6 +116,7 @@ logfile_bool : T_LOG T_OFF
 logfile_path : T_LOG T_PATH_VAL
 {
 	strncpy(conf.logfile, $2, FILENAME_MAXLEN);
+	free($2);
 };
 
 syslog_bool : T_SYSLOG T_ON
@@ -158,11 +159,13 @@ syslog_facility : T_SYSLOG T_STRING
 	    conf.syslog_facility != conf.stats.syslog_facility)
 		dlog(LOG_WARNING, "conflicting Syslog facility "
 		     "values, defaulting to General");
+	free($2);
 };
 
 lock : T_LOCK T_PATH_VAL
 {
 	strncpy(conf.lockfile, $2, FILENAME_MAXLEN);
+	free($2);
 };
 
 refreshtime : T_REFRESH T_NUMBER
@@ -257,6 +260,7 @@ multicast_option : T_IPV4_ADDR T_IP
 	}
 
 	conf.channel[conf.channel_num].u.mcast.ipproto = AF_INET;
+	free($2);
 };
 
 multicast_option : T_IPV6_ADDR T_IP
@@ -296,6 +300,7 @@ multicast_option : T_IPV6_ADDR T_IP
 		conf.channel[conf.channel_num].u.mcast.ifa.interface_index6 = idx;
 		conf.channel[conf.channel_num].u.mcast.ipproto = AF_INET6;
 	}
+	free($2);
 };
 
 multicast_option : T_IPV4_IFACE T_IP
@@ -315,11 +320,13 @@ multicast_option : T_IPV4_IFACE T_IP
 	}
 
 	conf.channel[conf.channel_num].u.mcast.ipproto = AF_INET;
+	free($2);
 };
 
 multicast_option : T_IPV6_IFACE T_IP
 {
 	dlog(LOG_WARNING, "`IPv6_interface' not required, ignoring");
+	free($2);
 }
 
 multicast_option : T_IFACE T_STRING
@@ -340,6 +347,7 @@ multicast_option : T_IFACE T_STRING
 		conf.channel[conf.channel_num].u.mcast.ifa.interface_index6 = idx;
 		conf.channel[conf.channel_num].u.mcast.ipproto = AF_INET6;
 	}
+	free($2);
 };
 
 multicast_option : T_GROUP T_NUMBER
@@ -414,6 +422,7 @@ udp_option : T_IPV4_ADDR T_IP
 		break;
 	}
 	conf.channel[conf.channel_num].u.udp.ipproto = AF_INET;
+	free($2);
 };
 
 udp_option : T_IPV6_ADDR T_IP
@@ -431,6 +440,7 @@ udp_option : T_IPV6_ADDR T_IP
 	break;
 #endif
 	conf.channel[conf.channel_num].u.udp.ipproto = AF_INET6;
+	free($2);
 };
 
 udp_option : T_IPV4_DEST_ADDR T_IP
@@ -459,6 +469,7 @@ udp_option : T_IPV6_DEST_ADDR T_IP
 	break;
 #endif
 	conf.channel[conf.channel_num].u.udp.ipproto = AF_INET6;
+	free($2);
 };
 
 udp_option : T_IFACE T_STRING
@@ -474,6 +485,7 @@ udp_option : T_IFACE T_STRING
 		break;
 	}
 	conf.channel[conf.channel_num].u.udp.server.ipv6.scope_id = idx;
+	free($2);
 };
 
 udp_option : T_PORT T_NUMBER
@@ -552,6 +564,7 @@ tcp_option : T_IPV4_ADDR T_IP
 		break;
 	}
 	conf.channel[conf.channel_num].u.tcp.ipproto = AF_INET;
+	free($2);
 };
 
 tcp_option : T_IPV6_ADDR T_IP
@@ -569,6 +582,7 @@ tcp_option : T_IPV6_ADDR T_IP
 	break;
 #endif
 	conf.channel[conf.channel_num].u.tcp.ipproto = AF_INET6;
+	free($2);
 };
 
 tcp_option : T_IPV4_DEST_ADDR T_IP
@@ -580,6 +594,7 @@ tcp_option : T_IPV4_DEST_ADDR T_IP
 		break;
 	}
 	conf.channel[conf.channel_num].u.tcp.ipproto = AF_INET;
+	free($2);
 };
 
 tcp_option : T_IPV6_DEST_ADDR T_IP
@@ -597,6 +612,7 @@ tcp_option : T_IPV6_DEST_ADDR T_IP
 	break;
 #endif
 	conf.channel[conf.channel_num].u.tcp.ipproto = AF_INET6;
+	free($2);
 };
 
 tcp_option : T_IFACE T_STRING
@@ -612,6 +628,7 @@ tcp_option : T_IFACE T_STRING
 		break;
 	}
 	conf.channel[conf.channel_num].u.tcp.server.ipv6.scope_id = idx;
+	free($2);
 };
 
 tcp_option : T_PORT T_NUMBER
@@ -669,6 +686,7 @@ unix_options:
 unix_option : T_PATH T_PATH_VAL
 {
 	strcpy(conf.local.path, $2);
+	free($2);
 };
 
 unix_option : T_BACKLOG T_NUMBER
@@ -757,6 +775,7 @@ expect_list:
 expect_item: T_STRING
 {
 	exp_filter_add(STATE(exp_filter), $1);
+	free($1);
 }
 
 sync_mode_alarm: T_SYNC_MODE T_ALARM '{' sync_mode_alarm_list '}'
@@ -993,6 +1012,7 @@ scheduler_line : T_TYPE T_STRING
 		dlog(LOG_ERR, "unknown scheduler `%s'", $2);
 		exit(EXIT_FAILURE);
 	}
+	free($2);
 };
 
 scheduler_line : T_PRIO T_NUMBER
@@ -1079,6 +1099,7 @@ filter_protocol_item : T_STRING
 	nfct_filter_add_attr_u32(STATE(filter),
 				 NFCT_FILTER_L4PROTO,
 				 pent->p_proto);
+	free($1);
 };
 
 filter_protocol_item : T_TCP
@@ -1209,6 +1230,7 @@ filter_address_item : T_IPV4_ADDR T_IP
 
 	nfct_filter_add_attr(STATE(filter), NFCT_FILTER_SRC_IPV4, &filter_ipv4);
 	nfct_filter_add_attr(STATE(filter), NFCT_FILTER_DST_IPV4, &filter_ipv4);
+	free($2);
 };
 
 filter_address_item : T_IPV6_ADDR T_IP
@@ -1268,6 +1290,7 @@ filter_address_item : T_IPV6_ADDR T_IP
 
 	nfct_filter_add_attr(STATE(filter), NFCT_FILTER_SRC_IPV6, &filter_ipv6);
 	nfct_filter_add_attr(STATE(filter), NFCT_FILTER_DST_IPV6, &filter_ipv6);
+	free($2);
 };
 
 filter_item : T_STATE T_ACCEPT '{' filter_state_list '}'
@@ -1330,6 +1353,7 @@ stat_logfile_bool : T_LOG T_OFF
 stat_logfile_path : T_LOG T_PATH_VAL
 {
 	strncpy(conf.stats.logfile, $2, FILENAME_MAXLEN);
+	free($2);
 };
 
 stat_syslog_bool : T_SYSLOG T_ON
@@ -1372,6 +1396,7 @@ stat_syslog_facility : T_SYSLOG T_STRING
 	    conf.stats.syslog_facility != conf.syslog_facility)
 		dlog(LOG_WARNING, "conflicting Syslog facility "
 		     "values, defaulting to General");
+	free($2);
 };
 
 helper: T_HELPER '{' helper_list '}'
@@ -1484,6 +1509,9 @@ helper_type: T_TYPE T_STRING T_STRING T_STRING '{' helper_type_list  '}'
 		}
 	}
 	list_add(&helper_inst->head, &CONFIG(cthelper).list);
+	free($2);
+	free($3);
+	free($4);
 };
 
 helper_type_list:
@@ -1534,6 +1562,7 @@ helper_type: T_HELPER_POLICY T_STRING '{' helper_policy_list '}'
 	/* Now object is complete. */
 	e->type = SYMBOL_HELPER_POLICY_EXPECT_ROOT;
 	stack_item_push(&symbol_stack, e);
+	free($2);
 };
 
 helper_policy_list:
-- 
2.7.4

Re: [PATCH 2/2 conntrack-tools] conntrackd: config: Free strdup()ed tokens

[Pablo Neira Ayuso]

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

On Fri, Jan 27, 2017 at 12:38:47PM -0800, Kevin Cernekee wrote:
> This frees T_IP, T_PATH_VAL, and T_STRING tokens.  They were being flagged
> by valgrind as memory leaks.

Thanks Kevin.

I think we can just remove the strdup() from the lexer, given that we
always copy these strings in the parser.

See patch attached.

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

diff --git a/src/read_config_lex.l b/src/read_config_lex.l
index 0282534e7291..a378269491f1 100644
--- a/src/read_config_lex.l
+++ b/src/read_config_lex.l
@@ -141,9 +141,9 @@ notrack		[N|n][O|o][T|t][R|r][A|a][C|c][K|k]
 {is_off}		{ return T_OFF; }
 {integer}		{ yylval.val = atoi(yytext); return T_NUMBER; }
 {signed_integer}	{ yylval.val = atoi(yytext); return T_SIGNED_NUMBER; }
-{ip4}			{ yylval.string = strdup(yytext); return T_IP; }
-{ip6}			{ yylval.string = strdup(yytext); return T_IP; }
-{path}			{ yylval.string = strdup(yytext); return T_PATH_VAL; }
+{ip4}			{ yylval.string = yytext; return T_IP; }
+{ip6}			{ yylval.string = yytext; return T_IP; }
+{path}			{ yylval.string = yytext; return T_PATH_VAL; }
 {alarm}			{ return T_ALARM; }
 {persistent}		{ dlog(LOG_WARNING, "Now `persistent' mode "
 			       "is called `alarm'. Please, update "
@@ -155,7 +155,7 @@ notrack		[N|n][O|o][T|t][R|r][A|a][C|c][K|k]
 			       "your conntrackd.conf file.\n");
 			  return T_FTFW; }
 {notrack}		{ return T_NOTRACK; }
-{string}		{ yylval.string = strdup(yytext); return T_STRING; }
+{string}		{ yylval.string = yytext; return T_STRING; }
 
 {comment}	;
 {ws}		;

Re: [PATCH 1/2 conntrack-tools] conntrackd: cthelper: Free pktb after use

[Pablo Neira Ayuso]

On Fri, Jan 27, 2017 at 12:38:46PM -0800, Kevin Cernekee wrote:
> According to valgrind, this currently leaks ~512B to 2kB for each
> packet sent to the userspace helper.

Applied, thanks!