| Message ID | 1497898444-116702-6-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Deferred |
| Headers | show |
>From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of >Bhanuprakash Bodireddy >Sent: Monday, June 19, 2017 7:54 PM >To: dev@openvswitch.org >Subject: [ovs-dev] [PATCH 6/6] netdev: Fix null pointer dereference reported by clang. > >Clang reports that array access from 'dumps' variable result in null pointer >dereference. > >Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> Hi Bhanu, LGTM - I also compiled this with gcc, clang, and sparse without issue. Checkpatch reports no obvious problems either. Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com> One thing - what version of clang are you using? My version (3.4.2) didn't detect any of the issues in this patchset. Alternatively, are there additional flags that you use when compiling with clang? Cheers, Mark >--- > lib/netdev.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > >diff --git a/lib/netdev.c b/lib/netdev.c >index 001b7b3..336c141 100644 >--- a/lib/netdev.c >+++ b/lib/netdev.c >@@ -2290,14 +2290,16 @@ netdev_ports_flow_dump_create(const void *obj, int *ports) > > dumps = count ? xzalloc(sizeof *dumps * count) : NULL; > >- HMAP_FOR_EACH(data, node, &port_to_netdev) { >- if (data->obj == obj) { >- if (netdev_flow_dump_create(data->netdev, &dumps[i])) { >- continue; >- } >+ if (dumps) { >+ HMAP_FOR_EACH(data, node, &port_to_netdev) { >+ if (data->obj == obj) { >+ if (netdev_flow_dump_create(data->netdev, &dumps[i])) { >+ continue; >+ } > >- dumps[i]->port = data->dpif_port.port_no; >- i++; >+ dumps[i]->port = data->dpif_port.port_no; >+ i++; >+ } > } > } > ovs_mutex_unlock(&netdev_hmap_mutex); >-- >2.4.11 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Mark, >> >>Clang reports that array access from 'dumps' variable result in null >>pointer dereference. >> >>Signed-off-by: Bhanuprakash Bodireddy >><bhanuprakash.bodireddy@intel.com> > >Hi Bhanu, > >LGTM - I also compiled this with gcc, clang, and sparse without issue. >Checkpatch reports no obvious problems either. > >Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > >One thing - what version of clang are you using? My version (3.4.2) didn't >detect any of the issues in this patchset. Alternatively, are there additional >flags that you use when compiling with clang? My clang version is 3.5.0. I was running clang static analyzer on my Keepalive branch to detect memory leaks and dead code and that's when I found these issues. - Bhanuprakash.
On Mon, Jun 19, 2017 at 07:54:04PM +0100, Bhanuprakash Bodireddy wrote: > Clang reports that array access from 'dumps' variable result in null pointer > dereference. > > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > --- > lib/netdev.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/lib/netdev.c b/lib/netdev.c > index 001b7b3..336c141 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2290,14 +2290,16 @@ netdev_ports_flow_dump_create(const void *obj, int *ports) > > dumps = count ? xzalloc(sizeof *dumps * count) : NULL; > > - HMAP_FOR_EACH(data, node, &port_to_netdev) { > - if (data->obj == obj) { > - if (netdev_flow_dump_create(data->netdev, &dumps[i])) { > - continue; > - } > + if (dumps) { > + HMAP_FOR_EACH(data, node, &port_to_netdev) { > + if (data->obj == obj) { > + if (netdev_flow_dump_create(data->netdev, &dumps[i])) { > + continue; > + } > > - dumps[i]->port = data->dpif_port.port_no; > - i++; > + dumps[i]->port = data->dpif_port.port_no; > + i++; > + } I think that, if 'dumps' is null, the code that uses it will never be reached, because we already saw in the previous loop through the hmap that the 'if' condition is never true, so I don't think that this change is needed.
diff --git a/lib/netdev.c b/lib/netdev.c index 001b7b3..336c141 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2290,14 +2290,16 @@ netdev_ports_flow_dump_create(const void *obj, int *ports) dumps = count ? xzalloc(sizeof *dumps * count) : NULL; - HMAP_FOR_EACH(data, node, &port_to_netdev) { - if (data->obj == obj) { - if (netdev_flow_dump_create(data->netdev, &dumps[i])) { - continue; - } + if (dumps) { + HMAP_FOR_EACH(data, node, &port_to_netdev) { + if (data->obj == obj) { + if (netdev_flow_dump_create(data->netdev, &dumps[i])) { + continue; + } - dumps[i]->port = data->dpif_port.port_no; - i++; + dumps[i]->port = data->dpif_port.port_no; + i++; + } } } ovs_mutex_unlock(&netdev_hmap_mutex);
Clang reports that array access from 'dumps' variable result in null pointer dereference. Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> --- lib/netdev.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)