kvargs: rework API to fix memory leak

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@6wind.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

diff --git a/lib/librte_kvargs/Makefile b/lib/librte_kvargs/Makefile
index 16f6107..b09359a 100644 (file)
--- a/lib/librte_kvargs/Makefile
+++ b/lib/librte_kvargs/Makefile
@@ -45,8 +45,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 b05a9bc..f368956 100644 (file)
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -34,24 +34,13 @@
 #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 3540e03..25e086f 100644 (file)
--- 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 3b2843d..c1c9b11 100644 (file)
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -670,30 +670,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;
@@ -705,13 +704,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);
        }
 
@@ -722,15 +721,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);
        }