| Message ID | 1423732089-6202-1-git-send-email-cunming.liang@intel.com (mailing list archive) |
|---|---|
| State | Rejected, 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 79C865A9A; Thu, 12 Feb 2015 10:08:20 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 44CE95A99 for <dev@dpdk.org>; Thu, 12 Feb 2015 10:08:18 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 12 Feb 2015 01:08:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,564,1418112000"; d="scan'208";a="651120136" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by orsmga001.jf.intel.com with ESMTP; 12 Feb 2015 01:08:16 -0800 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t1C98DOJ020625; Thu, 12 Feb 2015 17:08:13 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t1C98AEp006241; Thu, 12 Feb 2015 17:08:12 +0800 Received: (from cliang18@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t1C98ACi006237; Thu, 12 Feb 2015 17:08:10 +0800 From: Cunming Liang <cunming.liang@intel.com> To: dev@dpdk.org Date: Thu, 12 Feb 2015 17:08:09 +0800 Message-Id: <1423732089-6202-1-git-send-email-cunming.liang@intel.com> X-Mailer: git-send-email 1.7.4.1 Subject: [dpdk-dev] [PATCH v1] afpacket: fix critical issue reported by klocwork 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
Cunming Liang
Feb. 12, 2015, 9:08 a.m. UTC
Klocwork report 'req' might be used uninitialized.
In some cases it can 'goto error' when '*internals' not been set.
The result is unexpected checking the value of '*internals'.
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
lib/librte_pmd_af_packet/rte_eth_af_packet.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Comments
Hi Cunming, You would have more chance to have a review by CC'ing John. I checked your patch and have a comment below. 2015-02-12 17:08, Cunming Liang: > Klocwork report 'req' might be used uninitialized. > In some cases it can 'goto error' when '*internals' not been set. > The result is unexpected checking the value of '*internals'. > > Signed-off-by: Cunming Liang <cunming.liang@intel.com> > --- > lib/librte_pmd_af_packet/rte_eth_af_packet.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > index 1ffe1cd..185607d 100644 > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > size_t ifnamelen; > unsigned k_idx; > struct sockaddr_ll sockaddr; > - struct tpacket_req *req; > + struct tpacket_req *req = NULL; If *internals is set to NULL, there should be no case where req used and undefined. > struct pkt_rx_queue *rx_queue; > struct pkt_tx_queue *tx_queue; > int rc, qsockfd, tpver, discard; > unsigned int i, q, rdsize; > int fanout_arg __rte_unused, bypass __rte_unused; > > + *internals = NULL; > + > for (k_idx = 0; k_idx < kvlist->count; k_idx++) { > pair = &kvlist->pairs[k_idx]; > if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) >
On Fri, Feb 20, 2015 at 11:19:59AM +0100, Thomas Monjalon wrote: > Hi Cunming, > > You would have more chance to have a review by CC'ing John. > I checked your patch and have a comment below. > > 2015-02-12 17:08, Cunming Liang: > > Klocwork report 'req' might be used uninitialized. > > In some cases it can 'goto error' when '*internals' not been set. > > The result is unexpected checking the value of '*internals'. > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com> > > --- > > lib/librte_pmd_af_packet/rte_eth_af_packet.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > index 1ffe1cd..185607d 100644 > > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > > size_t ifnamelen; > > unsigned k_idx; > > struct sockaddr_ll sockaddr; > > - struct tpacket_req *req; > > + struct tpacket_req *req = NULL; > > If *internals is set to NULL, there should be no case where req used > and undefined. I agree -- it looks to me like req is protected by checking for *internals == NULL. I don't think this patch is necessary. > > struct pkt_rx_queue *rx_queue; > > struct pkt_tx_queue *tx_queue; > > int rc, qsockfd, tpver, discard; > > unsigned int i, q, rdsize; > > int fanout_arg __rte_unused, bypass __rte_unused; > > > > + *internals = NULL; > > + > > for (k_idx = 0; k_idx < kvlist->count; k_idx++) { > > pair = &kvlist->pairs[k_idx]; > > if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) > > > > > >
> -----Original Message----- > From: John W. Linville [mailto:linville@tuxdriver.com] > Sent: Saturday, February 21, 2015 2:39 AM > To: Thomas Monjalon > Cc: Liang, Cunming; dev@dpdk.org; John Linville > Subject: Re: [dpdk-dev] [PATCH v1] afpacket: fix critical issue reported by > klocwork > > On Fri, Feb 20, 2015 at 11:19:59AM +0100, Thomas Monjalon wrote: > > Hi Cunming, > > > > You would have more chance to have a review by CC'ing John. > > I checked your patch and have a comment below. > > > > 2015-02-12 17:08, Cunming Liang: > > > Klocwork report 'req' might be used uninitialized. > > > In some cases it can 'goto error' when '*internals' not been set. > > > The result is unexpected checking the value of '*internals'. > > > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com> > > > --- > > > lib/librte_pmd_af_packet/rte_eth_af_packet.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > index 1ffe1cd..185607d 100644 > > > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > > > size_t ifnamelen; > > > unsigned k_idx; > > > struct sockaddr_ll sockaddr; > > > - struct tpacket_req *req; > > > + struct tpacket_req *req = NULL; > > > > If *internals is set to NULL, there should be no case where req used > > and undefined. [LCM] Agree, so that's why I add '*internals = NULL' below as well. > > I agree -- it looks to me like req is protected by checking for > *internals == NULL. I don't think this patch is necessary. [LCM] The major piece of the patch is add setting for '*internals=NULL;'. > > > > struct pkt_rx_queue *rx_queue; > > > struct pkt_tx_queue *tx_queue; > > > int rc, qsockfd, tpver, discard; > > > unsigned int i, q, rdsize; > > > int fanout_arg __rte_unused, bypass __rte_unused; > > > > > > + *internals = NULL; > > > + > > > for (k_idx = 0; k_idx < kvlist->count; k_idx++) { > > > pair = &kvlist->pairs[k_idx]; > > > if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) > > > > > > > > > > > > > -- > John W. Linville Someday the world will need a hero, and you > linville@tuxdriver.com might be all we have. Be ready.
2015-02-25 00:57, Liang, Cunming: > From: John W. Linville [mailto:linville@tuxdriver.com] > > On Fri, Feb 20, 2015 at 11:19:59AM +0100, Thomas Monjalon wrote: > > > 2015-02-12 17:08, Cunming Liang: > > > > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > > > > size_t ifnamelen; > > > > unsigned k_idx; > > > > struct sockaddr_ll sockaddr; > > > > - struct tpacket_req *req; > > > > + struct tpacket_req *req = NULL; > > > > > > If *internals is set to NULL, there should be no case where req used > > > and undefined. > > [LCM] Agree, so that's why I add '*internals = NULL' below as well. > > > > I agree -- it looks to me like req is protected by checking for > > *internals == NULL. I don't think this patch is necessary. > > [LCM] The major piece of the patch is add setting for '*internals=NULL;'. Yes understood, but it is already initialized to NULL before calling rte_pmd_init_internals(): http://dpdk.org/browse/dpdk/tree/lib/librte_pmd_af_packet/rte_eth_af_packet.c#n706
> -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Wednesday, February 25, 2015 4:46 PM > To: Liang, Cunming > Cc: John W. Linville; dev@dpdk.org; John Linville > Subject: Re: [dpdk-dev] [PATCH v1] afpacket: fix critical issue reported by > klocwork > > 2015-02-25 00:57, Liang, Cunming: > > From: John W. Linville [mailto:linville@tuxdriver.com] > > > On Fri, Feb 20, 2015 at 11:19:59AM +0100, Thomas Monjalon wrote: > > > > 2015-02-12 17:08, Cunming Liang: > > > > > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > > > > > size_t ifnamelen; > > > > > unsigned k_idx; > > > > > struct sockaddr_ll sockaddr; > > > > > - struct tpacket_req *req; > > > > > + struct tpacket_req *req = NULL; > > > > > > > > If *internals is set to NULL, there should be no case where req used > > > > and undefined. > > > > [LCM] Agree, so that's why I add '*internals = NULL' below as well. > > > > > > I agree -- it looks to me like req is protected by checking for > > > *internals == NULL. I don't think this patch is necessary. > > > > [LCM] The major piece of the patch is add setting for '*internals=NULL;'. > > Yes understood, but it is already initialized to NULL before calling > rte_pmd_init_internals(): > http://dpdk.org/browse/dpdk/tree/lib/librte_pmd_af_packet/rte_eth_af_packet > .c#n706 [LCM] I see, it's complained by klocwork. So either adding 'internals=NULL' or adding some comments helps to avoid checking again on the next scanning. How do you think ?
2015-02-25 09:52, Liang, Cunming: > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > 2015-02-25 00:57, Liang, Cunming: > > > From: John W. Linville [mailto:linville@tuxdriver.com] > > > > On Fri, Feb 20, 2015 at 11:19:59AM +0100, Thomas Monjalon wrote: > > > > > 2015-02-12 17:08, Cunming Liang: > > > > > > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > > > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c > > > > > > @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, > > > > > > size_t ifnamelen; > > > > > > unsigned k_idx; > > > > > > struct sockaddr_ll sockaddr; > > > > > > - struct tpacket_req *req; > > > > > > + struct tpacket_req *req = NULL; > > > > > > > > > > If *internals is set to NULL, there should be no case where req used > > > > > and undefined. > > > > > > [LCM] Agree, so that's why I add '*internals = NULL' below as well. > > > > > > > > I agree -- it looks to me like req is protected by checking for > > > > *internals == NULL. I don't think this patch is necessary. > > > > > > [LCM] The major piece of the patch is add setting for '*internals=NULL;'. > > > > Yes understood, but it is already initialized to NULL before calling > > rte_pmd_init_internals(): > > http://dpdk.org/browse/dpdk/tree/lib/librte_pmd_af_packet/rte_eth_af_packet > > .c#n706 > [LCM] I see, it's complained by klocwork. > So either adding 'internals=NULL' or adding some comments helps to avoid checking again on the next scanning. > How do you think ? No, we don't have to pollute the code for a tool. You should check how to disable this false positive in your tool.
diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c index 1ffe1cd..185607d 100644 --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c @@ -439,13 +439,15 @@ rte_pmd_init_internals(const char *name, size_t ifnamelen; unsigned k_idx; struct sockaddr_ll sockaddr; - struct tpacket_req *req; + struct tpacket_req *req = NULL; struct pkt_rx_queue *rx_queue; struct pkt_tx_queue *tx_queue; int rc, qsockfd, tpver, discard; unsigned int i, q, rdsize; int fanout_arg __rte_unused, bypass __rte_unused; + *internals = NULL; + for (k_idx = 0; k_idx < kvlist->count; k_idx++) { pair = &kvlist->pairs[k_idx]; if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL)