[dpdk-stable] please check if patch 'eal: fix strdup usages in internal config' is applicable to LTS release 17.11.6
Yongseok Koh
yskoh at mellanox.com
Tue Mar 12 22:43:31 CET 2019
Okay will not merge it then.
Thanks,
Yongseok
> On Mar 11, 2019, at 3:08 AM, Burakov, Anatoly <anatoly.burakov at intel.com> wrote:
>
> It's not a critical bugfix, so it can be left out of it is deemed too much change for a stable release. However, it is indeed applicable to stable, and can be merged.
>
> Thanks,
> Anatoly
>
>
>> -----Original Message-----
>> From: Yongseok Koh [mailto:yskoh at mellanox.com]
>> Sent: Friday, March 8, 2019 6:09 PM
>> To: Burakov, Anatoly <anatoly.burakov at intel.com>
>> Cc: dpdk stable <stable at dpdk.org>
>> Subject: please check if patch 'eal: fix strdup usages in internal config' is
>> applicable to LTS release 17.11.6
>>
>> Hi,
>>
>> Even though the patch doesn't have "Cc: stable at dpdk.org" tag or "Fixes:"
>> tag, the commit log says it fixes some issue. Tags might be mistakenly missed.
>> We don't want to miss any fixes for stable releases. That's why I'm sending
>> this email.
>>
>> Please send backports if you think the patch should be merged to LTS release
>> 17.11.6 Or, let me know if you have any comments, say, need more time, or
>> it's worthless to backport it. And please send it to "stable at dpdk.org", but
>> not "dev at dpdk.org".
>>
>> FYI, branch 17.11 is located at tree:
>> git://dpdk.org/dpdk-stable
>>
>> It'd be great if you could do that in one or two weeks. Also, please add a
>> heading line like below before the commit log body:
>> [ backported from upstream commit xxx ]
>>
>> Example: https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpdk.org%2Fbrowse%2Fdpdk-&data=02%7C01%7Cyskoh%40mellanox.com%7C3d7486372a674d3f22a108d6a60974d8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636878956887750739&sdata=kYJAXumgJWJqqb%2BUSAkXdBusXo%2FEp%2FqL25txtHXH%2FHs%3D&reserved=0
>> stable/commit/?h=16.07&id=c4831394c7d1944d8ec27d52c22997f20d19718e
>>
>> Also please mention the target LTS in the subject line, as we have more than
>> one at the same time, for example:
>>
>> [PATCH 17.11] foo/bar: fix baz
>>
>> With git send-email, this can be achieved by appending the parameter:
>>
>> --subject-prefix='17.11'
>>
>>
>> Thanks.
>>
>> Yongseok
>>
>> ---
>> From 66d9f61de0885bd07662a016542600fe139d4eed Mon Sep 17 00:00:00
>> 2001
>> From: Anatoly Burakov <anatoly.burakov at intel.com>
>> Date: Thu, 10 Jan 2019 13:38:59 +0000
>> Subject: [PATCH] eal: fix strdup usages in internal config
>>
>> 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 <anatoly.burakov at intel.com>
>> ---
>> 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 c8e0da097..1ba9bd7cf 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
>> @@ -117,7 +117,7 @@ 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");
>> return -1;
>> @@ -535,9 +535,21 @@ 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);
>> exit(EXIT_SUCCESS);
>> @@ -923,6 +935,7 @@ 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
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>> @@ -169,6 +169,14 @@ eal_option_device_parse(void)
>> return ret;
>> }
>>
>> +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,7
>> +185,7 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
>> internal_cfg->memory = 0;
>> 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;
>> /* zero out the NUMA config */
>> @@ -1348,6 +1356,19 @@ 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) {
>> int i;
>> @@ -1387,7 +1408,7 @@ eal_check_common_options(struct internal_config
>> *internal_cfg)
>> RTE_LOG(ERR, EAL, "Invalid process type specified\n");
>> 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");
>> return -1;
>> 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
>> @@ -28,6 +28,10 @@ eal_create_runtime_dir(void); 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 *
>> eal_runtime_config_path(void)
>> @@ -89,7 +93,7 @@ static inline const char * 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 98e314fef..60eaead8f 100644
>> --- a/lib/librte_eal/common/eal_internal_cfg.h
>> +++ b/lib/librte_eal/common/eal_internal_cfg.h
>> @@ -66,9 +66,9 @@ struct internal_config {
>> volatile int syslog_facility; /**< facility passed to openlog() */
>> /** 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 */
>> struct hugepage_info hugepage_info[MAX_HUGEPAGE_SIZES];
>> diff --git a/lib/librte_eal/common/eal_options.h
>> b/lib/librte_eal/common/eal_options.h
>> index 1480c5d77..58ee9ae33 100644
>> --- a/lib/librte_eal/common/eal_options.h
>> +++ b/lib/librte_eal/common/eal_options.h
>> @@ -77,6 +77,7 @@ int eal_parse_common_option(int opt, const char
>> *argv,
>> struct internal_config *conf);
>> 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); enum rte_proc_type_t
>> eal_proc_type_detect(void); diff --git a/lib/librte_eal/linuxapp/eal/eal.c
>> b/lib/librte_eal/linuxapp/eal/eal.c
>> index 2d8d470b8..a386829f3 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -125,7 +125,7 @@ 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");
>> return -1;
>> @@ -727,13 +727,31 @@ eal_parse_args(int argc, char **argv)
>> exit(EXIT_SUCCESS);
>>
>> 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,
>> internal_config.socket_mem) < 0) {
>> @@ -783,10 +801,21 @@ eal_parse_args(int argc, char **argv)
>> break;
>>
>> 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 OPT_MATCH_ALLOCATIONS_NUM:
>> internal_config.match_allocations = 1;
>> break;
>> @@ -1238,6 +1267,7 @@ rte_eal_cleanup(void)
>> rte_memseg_walk(mark_freeable, NULL);
>> 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
>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>> @@ -438,7 +438,7 @@ 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 */
>> while (fgets(buf, sizeof(buf), f) != NULL) {
>> --
>> 2.11.0
More information about the stable
mailing list