| Message ID | 1425294562-26015-1-git-send-email-pawelx.wodkowski@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 861905A24; Mon, 2 Mar 2015 12:12:45 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 698E8595A for <dev@dpdk.org>; Mon, 2 Mar 2015 12:12:43 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 02 Mar 2015 03:10:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,675,1418112000"; d="scan'208";a="692638470" Received: from unknown (HELO Sent) ([10.217.248.233]) by orsmga002.jf.intel.com with SMTP; 02 Mar 2015 03:12:39 -0800 Received: by Sent (sSMTP sendmail emulation); Mon, 02 Mar 2015 12:09:25 +0100 From: Pawel Wodkowski <pawelx.wodkowski@intel.com> To: dev@dpdk.org Date: Mon, 2 Mar 2015 12:09:22 +0100 Message-Id: <1425294562-26015-1-git-send-email-pawelx.wodkowski@intel.com> X-Mailer: git-send-email 1.9.1 Subject: [dpdk-dev] [PATCH] eal: prevent dereferencing NULL pointer in rte_eal_devargs_add() X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Wodkowski, PawelX
March 2, 2015, 11:09 a.m. UTC
On failure devargs->args should not be accesed if devargs is NULL.
Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
lib/librte_eal/common/eal_common_devargs.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Comments
Hello Pawel, On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski <pawelx.wodkowski@intel.com > wrote: > On failure devargs->args should not be accesed if devargs is NULL. > accessed. > > Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com> > --- > 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@6wind.com>
On 3/2/15, 6:23 AM, "David Marchand" <david.marchand@6wind.com> wrote: >Hello Pawel, > >On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski ><pawelx.wodkowski@intel.com >> wrote: > >> On failure devargs->args should not be accesed if devargs is NULL. >> > >accessed. > > >> >> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com> >> --- >> 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@6wind.com> > >-- >David Marchand
On 2015-03-02 15:40, Wiles, Keith wrote: > > > On 3/2/15, 6:23 AM, "David Marchand" <david.marchand@6wind.com> wrote: > >> Hello Pawel, >> >> On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski >> <pawelx.wodkowski@intel.com >>> wrote: >> >>> On failure devargs->args should not be accesed if devargs is NULL. >>> >> >> accessed. >> >> >>> >>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com> >>> --- >>> 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
On 3/2/15, 8:55 AM, "Wodkowski, PawelX" <pawelx.wodkowski@intel.com> wrote: >On 2015-03-02 15:40, Wiles, Keith wrote: >> >> >> On 3/2/15, 6:23 AM, "David Marchand" <david.marchand@6wind.com> wrote: >> >>> Hello Pawel, >>> >>> On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski >>> <pawelx.wodkowski@intel.com >>>> wrote: >>> >>>> On failure devargs->args should not be accesed if devargs is NULL. >>>> >>> >>> accessed. >>> >>> >>>> >>>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com> >>>> --- >>>> 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
On 2015-03-02 17:47, Wiles, Keith wrote: > > > On 3/2/15, 8:55 AM, "Wodkowski, PawelX" <pawelx.wodkowski@intel.com> wrote: > >> On 2015-03-02 15:40, Wiles, Keith wrote: >>> >>> >>> On 3/2/15, 6:23 AM, "David Marchand" <david.marchand@6wind.com> wrote: >>> >>>> Hello Pawel, >>>> >>>> On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski >>>> <pawelx.wodkowski@intel.com >>>>> wrote: >>>> >>>>> On failure devargs->args should not be accesed if devargs is NULL. >>>>> >>>> >>>> accessed. >>>> >>>> >>>>> >>>>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com> >>>>> --- >>>>> 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 >
2015-03-02 13:23, David Marchand: > On Mon, Mar 2, 2015 at 12:09 PM, Pawel Wodkowski <pawelx.wodkowski@intel.com > > 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@6wind.com> Applied, thanks
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; }