[dpdk-dev] [PATCH 05/11] kvargs: rework API to fix memory leak

Olivier Matz olivier.matz at 6wind.com
Tue Jan 28 17:06:38 CET 2014


Before the patch, a call to rte_kvargs_tokenize() resulted in a call to
strdup() to allocate a modifiable copy of the argument string. This
string was never freed, excepted in the error cases of
rte_kvargs_tokenize() where rte_free() was wrongly called instead of
free(). In other cases, freeing this string was impossible as the
pointer not saved.

This patch introduces rte_kvargs_free() in order to free the structure
properly. The pointer to the duplicated string is now kept in the
rte_kvargs structure. A call to rte_kvargs_parse() directly allocates
the structure, making rte_kvargs_init() useless.

The only drawback of this API change is that a key/value associations
cannot be added to an existing kvlist. But it's not used today, and
there is not obvious use case for that.

Signed-off-by: Olivier Matz <olivier.matz at 6wind.com>
---
 lib/librte_kvargs/Makefile         |  3 +-
 lib/librte_kvargs/rte_kvargs.c     | 64 ++++++++++++++++++--------------------
 lib/librte_kvargs/rte_kvargs.h     | 42 +++++++++++--------------
 lib/librte_pmd_pcap/rte_eth_pcap.c | 27 ++++++++--------
 4 files changed, 63 insertions(+), 73 deletions(-)

diff --git a/lib/librte_kvargs/Makefile b/lib/librte_kvargs/Makefile
index c4c2855..69fd414 100644
--- a/lib/librte_kvargs/Makefile
+++ b/lib/librte_kvargs/Makefile
@@ -43,8 +43,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_KVARGS) := rte_kvargs.c
 INCS := rte_kvargs.h
 SYMLINK-$(CONFIG_RTE_LIBRTE_KVARGS)-include := $(INCS)
 
-# this lib needs eal and malloc
+# this lib needs eal
 DEPDIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += lib/librte_eal
-DEPDIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += lib/librte_malloc
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index 0bf46fe..9c52ce4 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -34,25 +34,14 @@
 #include <string.h>
 #include <sys/user.h>
 #include <linux/binfmts.h>
+#include <stdlib.h>
 
-#include <rte_malloc.h>
 #include <rte_log.h>
 #include <rte_string_fns.h>
 
 #include "rte_kvargs.h"
 
 /*
- * Initialize a rte_kvargs structure to an empty key/value list.
- */
-int
-rte_kvargs_init(struct rte_kvargs *kvlist)
-{
-	kvlist->count = 0;
-	memset(kvlist->pairs, 0, sizeof(kvlist->pairs));
-	return 0;
-}
-
-/*
  * Add a key-value pair at the end of a given key/value list.
  * Return an error if the list is full or if the key is duplicated.
  */
@@ -93,7 +82,6 @@ static int
 rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 {
 	unsigned i, count;
-	char *args;
 	char *pairs[RTE_KVARGS_MAX];
 	char *pair[2];
 
@@ -106,13 +94,13 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 	/* Copy the const char *params to a modifiable string
 	 * to pass to rte_strsplit
 	 */
-	args = strdup(params);
-	if(args == NULL){
+	kvlist->str = strdup(params);
+	if (kvlist->str == NULL) {
 		RTE_LOG(ERR, PMD, "Cannot parse arguments: not enough memory\n");
 		return -1;
 	}
 
-	count = rte_strsplit(args, strnlen(args, MAX_ARG_STRLEN), pairs,
+	count = rte_strsplit(kvlist->str, strnlen(kvlist->str, MAX_ARG_STRLEN), pairs,
 			RTE_KVARGS_MAX, RTE_KVARGS_PAIRS_DELIM);
 
 	for (i = 0; i < count; i++) {
@@ -127,17 +115,13 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 			RTE_LOG(ERR, PMD,
 				"Cannot parse arguments: wrong key or value\n"
 				"params=<%s>\n", params);
-			goto error;
+			return -1;
 		}
 
 		if (rte_kvargs_add_pair(kvlist, pair[0], pair[1]) < 0)
-			goto error;
+			return -1;
 	}
 	return 0;
-
-error:
-	rte_free(args);
-	return -1;
 }
 
 /*
@@ -222,25 +206,39 @@ rte_kvargs_process(const struct rte_kvargs *kvlist,
 	return 0;
 }
 
+/* free the rte_kvargs structure */
+void
+rte_kvargs_free(struct rte_kvargs *kvlist)
+{
+	if (kvlist->str != NULL)
+		free(kvlist->str);
+	free(kvlist);
+}
+
 /*
  * Parse the arguments "key=value;key=value;..." string and return
  * an allocated structure that contains a key/value list. Also
  * check if only valid keys were used.
  */
-int
-rte_kvargs_parse(struct rte_kvargs *kvlist,
-		const char *args,
-		const char *valid_keys[])
+struct rte_kvargs *
+rte_kvargs_parse(const char *args, const char *valid_keys[])
 {
+	struct rte_kvargs *kvlist;
 
-	int ret;
+	kvlist = malloc(sizeof(*kvlist));
+	if (kvlist == NULL)
+		return NULL;
+	memset(kvlist, 0, sizeof(*kvlist));
 
-	ret = rte_kvargs_tokenize(kvlist, args);
-	if (ret < 0)
-		return ret;
+	if (rte_kvargs_tokenize(kvlist, args) < 0) {
+		rte_kvargs_free(kvlist);
+		return NULL;
+	}
 
-	if (valid_keys == NULL)
-		return 0;
+	if (valid_keys != NULL && check_for_valid_keys(kvlist, valid_keys) < 0) {
+		rte_kvargs_free(kvlist);
+		return NULL;
+	}
 
-	return check_for_valid_keys(kvlist, valid_keys);
+	return kvlist;
 }
diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h
index 577f00b..47d78af 100644
--- a/lib/librte_kvargs/rte_kvargs.h
+++ b/lib/librte_kvargs/rte_kvargs.h
@@ -70,47 +70,41 @@ struct rte_kvargs_pair {
 
 /** Store a list of key/value associations */
 struct rte_kvargs {
+	char *str;      /**< copy of the argument string */
 	unsigned count; /**< number of entries in the list */
 	struct rte_kvargs_pair pairs[RTE_KVARGS_MAX]; /**< list of key/values */
 };
 
 /**
- * Initialize an empty rte_kvargs structure
+ * Allocate a rte_kvargs and store key/value associations from a string
  *
- * Set the content of the rte_kvargs structure given as a parameter
- * to an empty list of key/value.
+ * The function allocates and fills a rte_kvargs structure from a given
+ * string whose format is key1=value1;key2=value2;...
  *
- * @param kvlist
- *   The rte_kvargs structure
+ * The structure can be freed with rte_kvargs_free().
+ *
+ * @param args
+ *   The input string containing the key/value associations
+ * @param valid_keys
+ *   A list of valid keys (table of const char *, the last must be NULL).
+ *   This argument is ignored if NULL
  *
  * @return
- *   - 0 on success
- *   - Negative on error
+ *   - A pointer to an allocated rte_kvargs structure on success
+ *   - NULL on error
  */
-int rte_kvargs_init(struct rte_kvargs *kvlist);
+struct rte_kvargs *rte_kvargs_parse(const char *args, const char *valid_keys[]);
 
 /**
- * Append key/value associations in a rte_kvargs structure from a string
+ * Free a rte_kvargs structure
  *
- * The function fills a rte_kvargs structure from a string whose format
- * is key1=value1;key2=value2;...
- * The key/value associations are appended to those which are already
- * present in the rte_kvargs structure.
+ * Free a rte_kvargs structure previously allocated with
+ * rte_kvargs_parse().
  *
  * @param kvlist
  *   The rte_kvargs structure
- * @param args
- *   The input string containing the key/value associations
- * @param valid_keys
- *   A list of valid keys (table of const char *, the last must be NULL).
- *   This argument is ignored if NULL
- *
- * @return
- *   - 0 on success
- *   - Negative on error
  */
-int rte_kvargs_parse(struct rte_kvargs *kvlist,
-	const char *args, const char *valid_keys[]);
+void rte_kvargs_free(struct rte_kvargs *kvlist);
 
 /**
  * Call a handler function for each key/value matching the key
diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
index 2006b35..1f46cc9 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -702,30 +702,29 @@ rte_pmd_pcap_init(const char *name, const char *params)
 {
 	unsigned numa_node, using_dumpers = 0;
 	int ret;
-	struct rte_kvargs kvlist;
+	struct rte_kvargs *kvlist;
 	struct rx_pcaps pcaps;
 	struct tx_pcaps dumpers;
 
 	RTE_LOG(INFO, PMD, "Initializing pmd_pcap for %s\n", name);
 
-	rte_kvargs_init(&kvlist);
-
 	numa_node = rte_socket_id();
 
 	gettimeofday(&start_time, NULL);
 	start_cycles = rte_get_timer_cycles();
 	hz = rte_get_timer_hz();
 
-	if (rte_kvargs_parse(&kvlist, params, valid_arguments) < 0)
+	kvlist = rte_kvargs_parse(params, valid_arguments);
+	if (kvlist == NULL)
 		return -1;
 
 	/*
 	 * If iface argument is passed we open the NICs and use them for
 	 * reading / writing
 	 */
-	if (rte_kvargs_count(&kvlist, ETH_PCAP_IFACE_ARG) == 1) {
+	if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
 
-		ret = rte_kvargs_process(&kvlist, ETH_PCAP_IFACE_ARG,
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_IFACE_ARG,
 				&open_rx_tx_iface, &pcaps.pcaps[0]);
 		if (ret < 0)
 			return -1;
@@ -737,13 +736,13 @@ rte_pmd_pcap_init(const char *name, const char *params)
 	 * We check whether we want to open a RX stream from a real NIC or a
 	 * pcap file
 	 */
-	if ((pcaps.num_of_rx = rte_kvargs_count(&kvlist, ETH_PCAP_RX_PCAP_ARG))) {
-		ret = rte_kvargs_process(&kvlist, ETH_PCAP_RX_PCAP_ARG,
+	if ((pcaps.num_of_rx = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG))) {
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG,
 				&open_rx_pcap, &pcaps);
 	} else {
-		pcaps.num_of_rx = rte_kvargs_count(&kvlist,
+		pcaps.num_of_rx = rte_kvargs_count(kvlist,
 				ETH_PCAP_RX_IFACE_ARG);
-		ret = rte_kvargs_process(&kvlist, ETH_PCAP_RX_IFACE_ARG,
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_IFACE_ARG,
 				&open_rx_iface, &pcaps);
 	}
 
@@ -754,15 +753,15 @@ rte_pmd_pcap_init(const char *name, const char *params)
 	 * We check whether we want to open a TX stream to a real NIC or a
 	 * pcap file
 	 */
-	if ((dumpers.num_of_tx = rte_kvargs_count(&kvlist,
+	if ((dumpers.num_of_tx = rte_kvargs_count(kvlist,
 			ETH_PCAP_TX_PCAP_ARG))) {
-		ret = rte_kvargs_process(&kvlist, ETH_PCAP_TX_PCAP_ARG,
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_TX_PCAP_ARG,
 				&open_tx_pcap, &dumpers);
 		using_dumpers = 1;
 	} else {
-		dumpers.num_of_tx = rte_kvargs_count(&kvlist,
+		dumpers.num_of_tx = rte_kvargs_count(kvlist,
 				ETH_PCAP_TX_IFACE_ARG);
-		ret = rte_kvargs_process(&kvlist, ETH_PCAP_TX_IFACE_ARG,
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_TX_IFACE_ARG,
 				&open_tx_iface, &dumpers);
 	}
 
-- 
1.8.4.rc3



More information about the dev mailing list