Re: [PATCH] eal: prevent dereferencing NULL pointer in rte_eal_devargs_add()

[Pawel Wodkowski <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On 2015-03-02 17:47, Wiles, Keith wrote:
>
>
> On 3/2/15, 8:55 AM, "Wodkowski, PawelX" <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
>> On 2015-03-02 15:40, Wiles, Keith wrote:
>>>
>>>
>>> On 3/2/15, 6:23 AM, "David Marchand" <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> Hello Pawel,
>>>>
>>>> On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski
>>>> <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
>>>>> wrote:
>>>>
>>>>> On failure devargs->args should not be accesed if devargs is NULL.
>>>>>
>>>>
>>>> accessed.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>> ---
>>>>>    lib/librte_eal/common/eal_common_devargs.c | 7 ++++---
>>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_eal/common/eal_common_devargs.c
>>>>> b/lib/librte_eal/common/eal_common_devargs.c
>>>>> index 9b110f7..615945e 100644
>>>>> --- a/lib/librte_eal/common/eal_common_devargs.c
>>>>> +++ b/lib/librte_eal/common/eal_common_devargs.c
>>>>> @@ -124,12 +124,13 @@ rte_eal_devargs_add(enum rte_devtype devtype,
>>>>> const
>>>>> char *devargs_str)
>>>>>           return 0;
>>>>>
>>>>>    fail:
>>>>> -       if (devargs->args)
>>>>> -               free(devargs->args);
>>>>>           if (buf)
>>>>>                   free(buf);
>>>>> -       if (devargs)
>>>>> +       if (devargs) {
>>>>> +               free(devargs->args);
>>>
>>> Do you not still need to check for args being NULL before calling free?
>>
>> No, there is no need for that. The same for buf. This NOP check is
>> common practice in DPDK. I woul be good to clean this in whole library
>> in separate patch set.
>>
>> I recommend to read free() doc before doing another 'if (foo != NULL)
>> free(foo)'
>>
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html
>
> OK, did not realize this was changed. Do we know if all of the OSes DPDK
> is built supports this free style?
>
> I know that VxWorks did not support this free() method and I did port DPDK
> to that OS, but it is not a supported platform for DPDK.
>
> If some OS does not support passing NULL (and is supported by DPDK) to
> free, then we need to abstract the free into a macro to allow those
> systems to work correctly. I would expect using a macro for free would
> also help if all frees were reworked to not test for NULL.
>

This is standard C behaviour (since ANSI C?) and VxWorks claim to be 
compatible with it. If they lie, why bother?

> ++Keith
>>
>> --
>> Pawel
>


-- 
Pawel