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

Re: [dpdk-stable] [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-stable] [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-stable] [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-stable] [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