From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 12A431B455 for ; Thu, 31 Jan 2019 16:50:04 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B3F013A9E; Thu, 31 Jan 2019 15:50:03 +0000 (UTC) Received: from ktraynor.remote.csb (ovpn-117-200.ams2.redhat.com [10.36.117.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1D8875C21F; Thu, 31 Jan 2019 15:50:01 +0000 (UTC) From: Kevin Traynor To: Anatoly Burakov Cc: dpdk stable Date: Thu, 31 Jan 2019 15:48:25 +0000 Message-Id: <20190131154901.5383-17-ktraynor@redhat.com> In-Reply-To: <20190131154901.5383-1-ktraynor@redhat.com> References: <20190131154901.5383-1-ktraynor@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 31 Jan 2019 15:50:03 +0000 (UTC) Subject: [dpdk-stable] patch 'eal: fix strdup usages in internal config' has been queued to LTS release 18.11.1 X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Jan 2019 15:50:04 -0000 Hi, FYI, your patch has been queued to LTS release 18.11.1 Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. It will be pushed if I get no objections before 02/07/19. So please shout if anyone has objections. Also note that after the patch there's a diff of the upstream commit vs the patch applied to the branch. This will indicate if there was any rebasing needed to apply to the stable branch. If there were code changes for rebasing (ie: not only metadata diffs), please double check that the rebase was correctly done. Thanks. Kevin Traynor --- >>From bae45ffef2d71e077d76412ed40168e801730e2a Mon Sep 17 00:00:00 2001 From: Anatoly Burakov Date: Thu, 10 Jan 2019 13:38:59 +0000 Subject: [PATCH] eal: fix strdup usages in internal config [ upstream commit 66d9f61de0885bd07662a016542600fe139d4eed ] Currently, we use strdup in a few places to store command-line parameter values for certain internal config values. There are several issues with that. First of all, they're never freed, so memory ends up leaking either after EAL exit, or when these command-line options are supplied multiple times. Second of all, they're defined as `const char *`, so they *cannot* be freed even if we wanted to. Finally, strdup may return NULL, which will be stored in the config. For most fields, NULL is a valid value, but for the default prefix, the value is always expected to be valid. To fix all of this, three things are done. First, we change the definitions of these values to `char *` as opposed to `const char *`. This does not break the ABI, and previous code assumes constness (which is more restrictive), so it's safe to do so. Then, fix all usages of strdup to check return value, and add a cleanup function that will free the memory occupied by these strings, as well as freeing them before assigning a new value to prevent leaks when parameter is specified multiple times. And finally, add an internal API to query hugefile prefix, so that, absent of a valid value, a default value will be returned, and also fix up all usages of hugefile prefix to use this API instead of accessing hugefile prefix directly. Bugzilla ID: 108 Signed-off-by: Anatoly Burakov --- lib/librte_eal/bsdapp/eal/eal.c | 19 +++++++-- lib/librte_eal/common/eal_common_options.c | 25 +++++++++++- lib/librte_eal/common/eal_filesystem.h | 6 ++- lib/librte_eal/common/eal_internal_cfg.h | 6 +-- lib/librte_eal/common/eal_options.h | 1 + lib/librte_eal/linuxapp/eal/eal.c | 46 ++++++++++++++++++---- lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +- 7 files changed, 87 insertions(+), 18 deletions(-) diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index b6f96e0ab..f01495e33 100644 --- a/lib/librte_eal/bsdapp/eal/eal.c +++ b/lib/librte_eal/bsdapp/eal/eal.c @@ -116,5 +116,5 @@ eal_create_runtime_dir(void) /* create prefix-specific subdirectory under DPDK runtime dir */ ret = snprintf(runtime_dir, sizeof(runtime_dir), "%s/%s", - tmp, internal_config.hugefile_prefix); + tmp, eal_get_hugefile_prefix()); if (ret < 0 || ret == sizeof(runtime_dir)) { RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime path name\n"); @@ -458,7 +458,19 @@ eal_parse_args(int argc, char **argv) switch (opt) { case OPT_MBUF_POOL_OPS_NAME_NUM: - internal_config.user_mbuf_pool_ops_name = - strdup(optarg); + { + char *ops_name = strdup(optarg); + if (ops_name == NULL) + RTE_LOG(ERR, EAL, "Could not store mbuf pool ops name\n"); + else { + /* free old ops name */ + if (internal_config.user_mbuf_pool_ops_name != + NULL) + free(internal_config.user_mbuf_pool_ops_name); + + internal_config.user_mbuf_pool_ops_name = + ops_name; + } break; + } case 'h': eal_usage(prgname); @@ -843,4 +855,5 @@ rte_eal_cleanup(void) rte_service_finalize(); rte_mp_channel_cleanup(); + eal_cleanup_config(&internal_config); return 0; } diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c index e31eca5c0..bcf5f1b00 100644 --- a/lib/librte_eal/common/eal_common_options.c +++ b/lib/librte_eal/common/eal_common_options.c @@ -169,4 +169,12 @@ eal_option_device_parse(void) } +const char * +eal_get_hugefile_prefix(void) +{ + if (internal_config.hugefile_prefix != NULL) + return internal_config.hugefile_prefix; + return HUGEFILE_PREFIX_DEFAULT; +} + void eal_reset_internal_config(struct internal_config *internal_cfg) @@ -177,5 +185,5 @@ eal_reset_internal_config(struct internal_config *internal_cfg) internal_cfg->force_nrank = 0; internal_cfg->force_nchannel = 0; - internal_cfg->hugefile_prefix = HUGEFILE_PREFIX_DEFAULT; + internal_cfg->hugefile_prefix = NULL; internal_cfg->hugepage_dir = NULL; internal_cfg->force_sockets = 0; @@ -1347,4 +1355,17 @@ eal_auto_detect_cores(struct rte_config *cfg) } +int +eal_cleanup_config(struct internal_config *internal_cfg) +{ + if (internal_cfg->hugefile_prefix != NULL) + free(internal_cfg->hugefile_prefix); + if (internal_cfg->hugepage_dir != NULL) + free(internal_cfg->hugepage_dir); + if (internal_cfg->user_mbuf_pool_ops_name != NULL) + free(internal_cfg->user_mbuf_pool_ops_name); + + return 0; +} + int eal_adjust_config(struct internal_config *internal_cfg) @@ -1387,5 +1408,5 @@ eal_check_common_options(struct internal_config *internal_cfg) return -1; } - if (index(internal_cfg->hugefile_prefix, '%') != NULL) { + if (index(eal_get_hugefile_prefix(), '%') != NULL) { RTE_LOG(ERR, EAL, "Invalid char, '%%', in --"OPT_FILE_PREFIX" " "option\n"); diff --git a/lib/librte_eal/common/eal_filesystem.h b/lib/librte_eal/common/eal_filesystem.h index 64a028db7..89a3added 100644 --- a/lib/librte_eal/common/eal_filesystem.h +++ b/lib/librte_eal/common/eal_filesystem.h @@ -29,4 +29,8 @@ int eal_clean_runtime_dir(void); +/** Function to return hugefile prefix that's currently set up */ +const char * +eal_get_hugefile_prefix(void); + #define RUNTIME_CONFIG_FNAME "config" static inline const char * @@ -90,5 +94,5 @@ eal_get_hugefile_path(char *buffer, size_t buflen, const char *hugedir, int f_id { snprintf(buffer, buflen, HUGEFILE_FMT, hugedir, - internal_config.hugefile_prefix, f_id); + eal_get_hugefile_prefix(), f_id); buffer[buflen - 1] = '\0'; return buffer; diff --git a/lib/librte_eal/common/eal_internal_cfg.h b/lib/librte_eal/common/eal_internal_cfg.h index 737f17e35..783ce7de8 100644 --- a/lib/librte_eal/common/eal_internal_cfg.h +++ b/lib/librte_eal/common/eal_internal_cfg.h @@ -65,7 +65,7 @@ struct internal_config { /** default interrupt mode for VFIO */ volatile enum rte_intr_mode vfio_intr_mode; - const char *hugefile_prefix; /**< the base filename of hugetlbfs files */ - const char *hugepage_dir; /**< specific hugetlbfs directory to use */ - const char *user_mbuf_pool_ops_name; + char *hugefile_prefix; /**< the base filename of hugetlbfs files */ + char *hugepage_dir; /**< specific hugetlbfs directory to use */ + char *user_mbuf_pool_ops_name; /**< user defined mbuf pool ops name */ unsigned num_hugepage_sizes; /**< how many sizes on this system */ diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h index 5271f9449..327c95e96 100644 --- a/lib/librte_eal/common/eal_options.h +++ b/lib/librte_eal/common/eal_options.h @@ -76,4 +76,5 @@ int eal_parse_common_option(int opt, const char *argv, int eal_option_device_parse(void); int eal_adjust_config(struct internal_config *internal_cfg); +int eal_cleanup_config(struct internal_config *internal_cfg); int eal_check_common_options(struct internal_config *internal_cfg); void eal_common_usage(void); diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c index bcea22463..0226b7f6a 100644 --- a/lib/librte_eal/linuxapp/eal/eal.c +++ b/lib/librte_eal/linuxapp/eal/eal.c @@ -126,5 +126,5 @@ eal_create_runtime_dir(void) /* create prefix-specific subdirectory under DPDK runtime dir */ ret = snprintf(runtime_dir, sizeof(runtime_dir), "%s/%s", - tmp, internal_config.hugefile_prefix); + tmp, eal_get_hugefile_prefix()); if (ret < 0 || ret == sizeof(runtime_dir)) { RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime path name\n"); @@ -727,11 +727,29 @@ eal_parse_args(int argc, char **argv) case OPT_HUGE_DIR_NUM: - internal_config.hugepage_dir = strdup(optarg); + { + char *hdir = strdup(optarg); + if (hdir == NULL) + RTE_LOG(ERR, EAL, "Could not store hugepage directory\n"); + else { + /* free old hugepage dir */ + if (internal_config.hugepage_dir != NULL) + free(internal_config.hugepage_dir); + internal_config.hugepage_dir = hdir; + } break; - + } case OPT_FILE_PREFIX_NUM: - internal_config.hugefile_prefix = strdup(optarg); + { + char *prefix = strdup(optarg); + if (prefix == NULL) + RTE_LOG(ERR, EAL, "Could not store file prefix\n"); + else { + /* free old prefix */ + if (internal_config.hugefile_prefix != NULL) + free(internal_config.hugefile_prefix); + internal_config.hugefile_prefix = prefix; + } break; - + } case OPT_SOCKET_MEM_NUM: if (eal_parse_socket_arg(optarg, @@ -783,8 +801,19 @@ eal_parse_args(int argc, char **argv) case OPT_MBUF_POOL_OPS_NAME_NUM: - internal_config.user_mbuf_pool_ops_name = - strdup(optarg); - break; + { + char *ops_name = strdup(optarg); + if (ops_name == NULL) + RTE_LOG(ERR, EAL, "Could not store mbuf pool ops name\n"); + else { + /* free old ops name */ + if (internal_config.user_mbuf_pool_ops_name != + NULL) + free(internal_config.user_mbuf_pool_ops_name); + internal_config.user_mbuf_pool_ops_name = + ops_name; + } + break; + } default: if (opt < OPT_LONG_MIN_NUM && isprint(opt)) { @@ -1231,4 +1260,5 @@ rte_eal_cleanup(void) rte_service_finalize(); rte_mp_channel_cleanup(); + eal_cleanup_config(&internal_config); return 0; } diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c index 32feb415d..e05da74ca 100644 --- a/lib/librte_eal/linuxapp/eal/eal_memory.c +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c @@ -435,5 +435,5 @@ find_numasocket(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi) snprintf(hugedir_str, sizeof(hugedir_str), - "%s/%s", hpi->hugedir, internal_config.hugefile_prefix); + "%s/%s", hpi->hugedir, eal_get_hugefile_prefix()); /* parse numa map */ -- 2.19.0 --- Diff of the applied patch vs upstream commit (please double-check if non-empty: --- --- - 2019-01-31 15:44:06.143716658 +0000 +++ 0017-eal-fix-strdup-usages-in-internal-config.patch 2019-01-31 15:44:05.000000000 +0000 @@ -1,8 +1,10 @@ -From 66d9f61de0885bd07662a016542600fe139d4eed Mon Sep 17 00:00:00 2001 +From bae45ffef2d71e077d76412ed40168e801730e2a Mon Sep 17 00:00:00 2001 From: Anatoly Burakov Date: Thu, 10 Jan 2019 13:38:59 +0000 Subject: [PATCH] eal: fix strdup usages in internal config +[ upstream commit 66d9f61de0885bd07662a016542600fe139d4eed ] + Currently, we use strdup in a few places to store command-line parameter values for certain internal config values. There are several issues with that. @@ -49,17 +51,17 @@ 7 files changed, 87 insertions(+), 18 deletions(-) diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c -index c8e0da097..1ba9bd7cf 100644 +index b6f96e0ab..f01495e33 100644 --- a/lib/librte_eal/bsdapp/eal/eal.c +++ b/lib/librte_eal/bsdapp/eal/eal.c -@@ -118,5 +118,5 @@ eal_create_runtime_dir(void) +@@ -116,5 +116,5 @@ eal_create_runtime_dir(void) /* create prefix-specific subdirectory under DPDK runtime dir */ ret = snprintf(runtime_dir, sizeof(runtime_dir), "%s/%s", - tmp, internal_config.hugefile_prefix); + tmp, eal_get_hugefile_prefix()); if (ret < 0 || ret == sizeof(runtime_dir)) { RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime path name\n"); -@@ -536,7 +536,19 @@ eal_parse_args(int argc, char **argv) +@@ -458,7 +458,19 @@ eal_parse_args(int argc, char **argv) switch (opt) { case OPT_MBUF_POOL_OPS_NAME_NUM: - internal_config.user_mbuf_pool_ops_name = @@ -81,17 +83,17 @@ + } case 'h': eal_usage(prgname); -@@ -924,4 +936,5 @@ rte_eal_cleanup(void) +@@ -843,4 +855,5 @@ rte_eal_cleanup(void) rte_service_finalize(); rte_mp_channel_cleanup(); + eal_cleanup_config(&internal_config); return 0; } diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c -index 6e3a83b98..a2d862b5f 100644 +index e31eca5c0..bcf5f1b00 100644 --- a/lib/librte_eal/common/eal_common_options.c +++ b/lib/librte_eal/common/eal_common_options.c -@@ -170,4 +170,12 @@ eal_option_device_parse(void) +@@ -169,4 +169,12 @@ eal_option_device_parse(void) } +const char * @@ -104,14 +106,14 @@ + void eal_reset_internal_config(struct internal_config *internal_cfg) -@@ -178,5 +186,5 @@ eal_reset_internal_config(struct internal_config *internal_cfg) +@@ -177,5 +185,5 @@ eal_reset_internal_config(struct internal_config *internal_cfg) internal_cfg->force_nrank = 0; internal_cfg->force_nchannel = 0; - internal_cfg->hugefile_prefix = HUGEFILE_PREFIX_DEFAULT; + internal_cfg->hugefile_prefix = NULL; internal_cfg->hugepage_dir = NULL; internal_cfg->force_sockets = 0; -@@ -1348,4 +1356,17 @@ eal_auto_detect_cores(struct rte_config *cfg) +@@ -1347,4 +1355,17 @@ eal_auto_detect_cores(struct rte_config *cfg) } +int @@ -129,7 +131,7 @@ + int eal_adjust_config(struct internal_config *internal_cfg) -@@ -1388,5 +1409,5 @@ eal_check_common_options(struct internal_config *internal_cfg) +@@ -1387,5 +1408,5 @@ eal_check_common_options(struct internal_config *internal_cfg) return -1; } - if (index(internal_cfg->hugefile_prefix, '%') != NULL) { @@ -157,10 +159,10 @@ buffer[buflen - 1] = '\0'; return buffer; diff --git a/lib/librte_eal/common/eal_internal_cfg.h b/lib/librte_eal/common/eal_internal_cfg.h -index 98e314fef..60eaead8f 100644 +index 737f17e35..783ce7de8 100644 --- a/lib/librte_eal/common/eal_internal_cfg.h +++ b/lib/librte_eal/common/eal_internal_cfg.h -@@ -67,7 +67,7 @@ struct internal_config { +@@ -65,7 +65,7 @@ struct internal_config { /** default interrupt mode for VFIO */ volatile enum rte_intr_mode vfio_intr_mode; - const char *hugefile_prefix; /**< the base filename of hugetlbfs files */ @@ -172,17 +174,17 @@ /**< user defined mbuf pool ops name */ unsigned num_hugepage_sizes; /**< how many sizes on this system */ diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h -index 1480c5d77..58ee9ae33 100644 +index 5271f9449..327c95e96 100644 --- a/lib/librte_eal/common/eal_options.h +++ b/lib/librte_eal/common/eal_options.h -@@ -78,4 +78,5 @@ int eal_parse_common_option(int opt, const char *argv, +@@ -76,4 +76,5 @@ int eal_parse_common_option(int opt, const char *argv, int eal_option_device_parse(void); int eal_adjust_config(struct internal_config *internal_cfg); +int eal_cleanup_config(struct internal_config *internal_cfg); int eal_check_common_options(struct internal_config *internal_cfg); void eal_common_usage(void); diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c -index 2d8d470b8..a386829f3 100644 +index bcea22463..0226b7f6a 100644 --- a/lib/librte_eal/linuxapp/eal/eal.c +++ b/lib/librte_eal/linuxapp/eal/eal.c @@ -126,5 +126,5 @@ eal_create_runtime_dir(void) @@ -192,7 +194,7 @@ + tmp, eal_get_hugefile_prefix()); if (ret < 0 || ret == sizeof(runtime_dir)) { RTE_LOG(ERR, EAL, "Error creating prefix-specific runtime path name\n"); -@@ -728,11 +728,29 @@ eal_parse_args(int argc, char **argv) +@@ -727,11 +727,29 @@ eal_parse_args(int argc, char **argv) case OPT_HUGE_DIR_NUM: - internal_config.hugepage_dir = strdup(optarg); @@ -226,7 +228,7 @@ + } case OPT_SOCKET_MEM_NUM: if (eal_parse_socket_arg(optarg, -@@ -784,8 +802,19 @@ eal_parse_args(int argc, char **argv) +@@ -783,8 +801,19 @@ eal_parse_args(int argc, char **argv) case OPT_MBUF_POOL_OPS_NAME_NUM: - internal_config.user_mbuf_pool_ops_name = @@ -247,19 +249,19 @@ + } + break; + } - case OPT_MATCH_ALLOCATIONS_NUM: - internal_config.match_allocations = 1; -@@ -1239,4 +1268,5 @@ rte_eal_cleanup(void) + default: + if (opt < OPT_LONG_MIN_NUM && isprint(opt)) { +@@ -1231,4 +1260,5 @@ rte_eal_cleanup(void) rte_service_finalize(); rte_mp_channel_cleanup(); + eal_cleanup_config(&internal_config); return 0; } diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c -index 7d922a965..1b96b576e 100644 +index 32feb415d..e05da74ca 100644 --- a/lib/librte_eal/linuxapp/eal/eal_memory.c +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c -@@ -439,5 +439,5 @@ find_numasocket(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi) +@@ -435,5 +435,5 @@ find_numasocket(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi) snprintf(hugedir_str, sizeof(hugedir_str), - "%s/%s", hpi->hugedir, internal_config.hugefile_prefix);