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 <[email protected]>
> ---
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
