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

["Wiles, Keith" <keith.wiles-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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.

++Keith
>
>-- 
>Pawel