Re: [dpdk-stable] please check if patch 'eal: fix strdup usages in internal config' is applicable to LTS release 17.11.6

[Yongseok Koh <yskoh@mellanox.com>]

Okay will not merge it then.


Thanks,
Yongseok

> On Mar 11, 2019, at 3:08 AM, Burakov, Anatoly <anatoly.burakov@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@mellanox.com]
>> Sent: Friday, March 8, 2019 6:09 PM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Cc: dpdk stable <stable@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@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@dpdk.org", but
>> not "dev@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-&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C3d7486372a674d3f22a108d6a60974d8%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636878956887750739&amp;sdata=kYJAXumgJWJqqb%2BUSAkXdBusXo%2FEp%2FqL25txtHXH%2FHs%3D&amp;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@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@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