| Message ID | 20240516193700.212737-2-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clang: Fix Clang's static analyzer detections. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 80. Subject: netdev-offload: Fix Clang's static analyzer 'null pointer dereference' warnings. Lines checked: 41, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 5/16/24 21:36, Mike Pattrick wrote: > Clang's static analyzer will complain about a null pointer dereference > because dumps can be set to null and then there is a loop where it could > have been written to. > > Instead, return early from the netdev_ports_flow_dump_create function if > dumps is NULL. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-offload.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) Thanks for the patch! It is a false-positive as both loops are iterated while holding a lock. Might be worth mentioning in the commit message. Also, please, use more precise and concise names for the patches in the set to reduce the chance of commits with the exact same names in the git history. This may confuse some automation. This one, for example, can be named: netdev-offload: Fix null pointer dereference warning on dump creation. There is no need to mention clang or static analysis in the subject, this can be put in the commit message. > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index 931d634e1..02b1cf203 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -638,7 +638,14 @@ netdev_ports_flow_dump_create(const char *dpif_type, int *ports, bool terse) > } > } > > - dumps = count ? xzalloc(sizeof *dumps * count) : NULL; > + if (count == 0) { I'd prefer !count for zero checks. > + *ports = 0; > + ovs_rwlock_unlock(&port_to_netdev_rwlock); > + > + return NULL; > + } > + > + dumps = xzalloc(sizeof *dumps * count); I'd suggest we initialize the 'dumps' to NULL at the top and then just if (!count) { goto unlock; } to avoid code duplication. Best regards, Ilya Maximets.
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 931d634e1..02b1cf203 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -638,7 +638,14 @@ netdev_ports_flow_dump_create(const char *dpif_type, int *ports, bool terse) } } - dumps = count ? xzalloc(sizeof *dumps * count) : NULL; + if (count == 0) { + *ports = 0; + ovs_rwlock_unlock(&port_to_netdev_rwlock); + + return NULL; + } + + dumps = xzalloc(sizeof *dumps * count); HMAP_FOR_EACH (data, portno_node, &port_to_netdev) { if (netdev_get_dpif_type(data->netdev) == dpif_type) {
Clang's static analyzer will complain about a null pointer dereference because dumps can be set to null and then there is a loop where it could have been written to. Instead, return early from the netdev_ports_flow_dump_create function if dumps is NULL. Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-offload.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)