[dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Anatoly Burakov]

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 */
@@ -1347,6 +1355,19 @@ eal_auto_detect_cores(struct rte_config *cfg)
 	cfg->lcore_count -= removed;
 }
 
+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,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);
-			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;
+		}
 		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.17.1

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Thomas Monjalon]

10/01/2019 14:38, Anatoly Burakov:
> 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>

Applied, thanks

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Kevin Traynor]

On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
> 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
> 

Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
was reported on 17.11. Is it for backport to stable branches?

> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Burakov, Anatoly]

On 31-Jan-19 11:21 AM, Kevin Traynor wrote:
> On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
>> 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
>>
> 
> Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
> was reported on 17.11. Is it for backport to stable branches?
> 

It can be. Whether it's worth the effort of backporting is not my call :)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Kevin Traynor]

On 01/31/2019 02:10 PM, Burakov, Anatoly wrote:
> On 31-Jan-19 11:21 AM, Kevin Traynor wrote:
>> On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
>>> 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
>>>
>>
>> Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
>> was reported on 17.11. Is it for backport to stable branches?
>>
> 
> It can be. Whether it's worth the effort of backporting is not my call :)
> 

It's fine for 18.11 branch anyway, just needed a little help due to some
changed context. I will send diff to stable list as normal.

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Thomas Monjalon]

31/01/2019 15:15, Kevin Traynor:
> On 01/31/2019 02:10 PM, Burakov, Anatoly wrote:
> > On 31-Jan-19 11:21 AM, Kevin Traynor wrote:
> >> On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
> >>> 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
> >>>
> >>
> >> Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
> >> was reported on 17.11. Is it for backport to stable branches?
> >>
> > 
> > It can be. Whether it's worth the effort of backporting is not my call :)
> > 
> 
> It's fine for 18.11 branch anyway, just needed a little help due to some
> changed context. I will send diff to stable list as normal.

Nothing was broken. I see it like an improvement.
Not sure it is worth the effort.

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Burakov, Anatoly]

On 31-Jan-19 3:04 PM, Thomas Monjalon wrote:
> 31/01/2019 15:15, Kevin Traynor:
>> On 01/31/2019 02:10 PM, Burakov, Anatoly wrote:
>>> On 31-Jan-19 11:21 AM, Kevin Traynor wrote:
>>>> On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
>>>>> 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
>>>>>
>>>>
>>>> Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
>>>> was reported on 17.11. Is it for backport to stable branches?
>>>>
>>>
>>> It can be. Whether it's worth the effort of backporting is not my call :)
>>>
>>
>> It's fine for 18.11 branch anyway, just needed a little help due to some
>> changed context. I will send diff to stable list as normal.
> 
> Nothing was broken. I see it like an improvement.
> Not sure it is worth the effort.
> 

Well, *technically*, there was a memory leak. For example, if you 
specify mbuf pool ops flag multiple times, previously allocated strdup() 
call results would be discarded and leaked.

However, it's such a minor issue that it's indeed likely not worth the 
effort.


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal: fix strdup usages in internal config

[Kevin Traynor]

On 01/31/2019 03:55 PM, Burakov, Anatoly wrote:
> On 31-Jan-19 3:04 PM, Thomas Monjalon wrote:
>> 31/01/2019 15:15, Kevin Traynor:
>>> On 01/31/2019 02:10 PM, Burakov, Anatoly wrote:
>>>> On 31-Jan-19 11:21 AM, Kevin Traynor wrote:
>>>>> On 01/10/2019 01:38 PM, Anatoly Burakov wrote:
>>>>>> 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
>>>>>>
>>>>>
>>>>> Hi Anatoly - this doesn't have stable or Fixes tags, but the bugzilla
>>>>> was reported on 17.11. Is it for backport to stable branches?
>>>>>
>>>>
>>>> It can be. Whether it's worth the effort of backporting is not my
>>>> call :)
>>>>
>>>
>>> It's fine for 18.11 branch anyway, just needed a little help due to some
>>> changed context. I will send diff to stable list as normal.
>>
>> Nothing was broken. I see it like an improvement.
>> Not sure it is worth the effort.
>>
> 
> Well, *technically*, there was a memory leak. For example, if you
> specify mbuf pool ops flag multiple times, previously allocated strdup()
> call results would be discarded and leaked.
> 
> However, it's such a minor issue that it's indeed likely not worth the
> effort.
> 
> 

It's already done - just sent it in the batch a few mins ago