From ef8e5447e3a63dd2a0fc025a143aa7bd83d0e602 Mon Sep 17 00:00:00 2001 From: Olivier Matz Date: Tue, 28 Jan 2014 17:06:38 +0100 Subject: [PATCH] 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 Acked-by: Bruce Richardson --- 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 16f610795e..b09359a500 100644 --- 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 b05a9bc88a..f3689564d8 100644 --- a/lib/librte_kvargs/rte_kvargs.c +++ b/lib/librte_kvargs/rte_kvargs.c @@ -34,24 +34,13 @@ #include #include #include +#include -#include #include #include #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 3540e034ca..25e086f01d 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 3b2843d25c..c1c9b11905 100644 --- 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); } -- 2.20.1