diff mbox series

[ovs-dev] ofproto-dpif-xlate: Remove misleading wc NULL check in packet mirror.

Message ID 20240716124718.455536-1-i.maximets@ovn.org
State Accepted
Commit f973d9543ecd461adcb63ca8db08a51252f1a8a7
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] ofproto-dpif-xlate: Remove misleading wc NULL check in packet mirror. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Ilya Maximets July 16, 2024, 12:47 p.m. UTC
'wc' can't be NULL there and if it can we'd already crash a few lines
before setting up vlan flags.

The check is misleading as it makes people to assume that wc can be
NULL.  And it makes Coverity think the same:

  CID 1596572: (#1 of 1): Dereference after null check (FORWARD_NULL)
  25. var_deref_op: Dereferencing null pointer ctx->wc.

  14. var_compare_op: Comparing ctx->wc to null implies that ctx->wc
      might be null

Remove the check.

Fixes: 3b1882261c8b ("ofproto-dpif-mirror: Add support for pre-selection filter.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mike Pattrick July 16, 2024, 1:12 p.m. UTC | #1
On Tue, Jul 16, 2024 at 8:47 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> 'wc' can't be NULL there and if it can we'd already crash a few lines
> before setting up vlan flags.
>
> The check is misleading as it makes people to assume that wc can be
> NULL.  And it makes Coverity think the same:
>
>   CID 1596572: (#1 of 1): Dereference after null check (FORWARD_NULL)
>   25. var_deref_op: Dereferencing null pointer ctx->wc.
>
>   14. var_compare_op: Comparing ctx->wc to null implies that ctx->wc
>       might be null
>
> Remove the check.
>
> Fixes: 3b1882261c8b ("ofproto-dpif-mirror: Add support for pre-selection filter.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

It looks like you're right, it can't be null.

Acked-by: Mike Pattrick <mkp@redhat.com>
Ilya Maximets July 17, 2024, 9:48 p.m. UTC | #2
On 7/16/24 15:12, Mike Pattrick wrote:
> On Tue, Jul 16, 2024 at 8:47 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> 'wc' can't be NULL there and if it can we'd already crash a few lines
>> before setting up vlan flags.
>>
>> The check is misleading as it makes people to assume that wc can be
>> NULL.  And it makes Coverity think the same:
>>
>>   CID 1596572: (#1 of 1): Dereference after null check (FORWARD_NULL)
>>   25. var_deref_op: Dereferencing null pointer ctx->wc.
>>
>>   14. var_compare_op: Comparing ctx->wc to null implies that ctx->wc
>>       might be null
>>
>> Remove the check.
>>
>> Fixes: 3b1882261c8b ("ofproto-dpif-mirror: Add support for pre-selection filter.")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>> ---
> 
> It looks like you're right, it can't be null.
> 
> Acked-by: Mike Pattrick <mkp@redhat.com>
> 

Thanks, Mike!  Applied and backported to 3.4.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index be2c70721..02567a961 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2315,7 +2315,7 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         }
 
         /* After the VLAN check, apply a flow mask if a filter is specified. */
-        if (ctx->wc && mc.filter_flow) {
+        if (mc.filter_flow) {
             flow_wildcards_union_with_minimask(ctx->wc, mc.filter_mask);
             if (!OVS_UNLIKELY(
                  miniflow_equal_flow_in_minimask(mc.filter_flow,