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

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

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