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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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: http://dpdk.org/browse/dpdk-
> 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