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

[Kevin Traynor <ktraynor@redhat.com>]

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