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

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

[Pawel Wodkowski]

On failure devargs->args should not be accesed if devargs is NULL.

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);
 		free(devargs);
+	}
+
 	return -1;
 }
 
-- 
1.9.1

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

[David Marchand]

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);
>                 free(devargs);
> +       }
> +
>         return -1;
>  }
>
>
Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
Acked-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

-- 
David Marchand

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

[Wiles, Keith]

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?
>>                 free(devargs);
>> +       }
>> +
>>         return -1;
>>  }
>>
>>
>Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
>Acked-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
>
>-- 
>David Marchand

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

[Pawel Wodkowski]

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

-- 
Pawel

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

[Wiles, Keith]

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

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

[Pawel Wodkowski]

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

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

[Thomas Monjalon]

2015-03-02 13:23, David Marchand:
> 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.
> 
> Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
> Acked-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

Applied, thanks