[ovs-dev] pinctrl: Avoid false positive out of bounds warning.

GCC reported an out of bounds array access when compiling with -O2:
  controller/pinctrl.c: In function ‘pinctrl_handle_icmp’:
  controller/pinctrl.c:1673:9: error: ‘memcpy’ offset [0, 19] is out of the bounds [0, 0] [-Werror=array-bounds]
   1673 |         memcpy(data, in_ip, in_ip_len);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

GCC version is: 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC)

This happens because the compiler cannot infer that in this specific
case dp_packet_l4(&packet) can never return NULL.

pinctrl_compose_ipv4() calls eth_compose() which calls
dp_packet_set_l3() and always sets 'l4_ofs' to a value that's different
from UINT16_MAX.

Help the compiler out by asserting that the L4 data is not NULL.

Simplified code sample shared on the gcc-help mailing list:
https://gcc.gnu.org/pipermail/gcc-help/2022-January/141160.html

Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
Tested-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/pinctrl.c | 6 ++++++
 1 file changed, 6 insertions(+)

Acked-by: Mark Michelson <mmichels@redhat.com>

On 1/28/22 11:14, Dumitru Ceara wrote:
> GCC reported an out of bounds array access when compiling with -O2:
>    controller/pinctrl.c: In function ‘pinctrl_handle_icmp’:
>    controller/pinctrl.c:1673:9: error: ‘memcpy’ offset [0, 19] is out of the bounds [0, 0] [-Werror=array-bounds]
>     1673 |         memcpy(data, in_ip, in_ip_len);
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> GCC version is: 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC)
> 
> This happens because the compiler cannot infer that in this specific
> case dp_packet_l4(&packet) can never return NULL.
> 
> pinctrl_compose_ipv4() calls eth_compose() which calls
> dp_packet_set_l3() and always sets 'l4_ofs' to a value that's different
> from UINT16_MAX.
> 
> Help the compiler out by asserting that the L4 data is not NULL.
> 
> Simplified code sample shared on the gcc-help mailing list:
> https://gcc.gnu.org/pipermail/gcc-help/2022-January/141160.html
> 
> Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
> Tested-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>   controller/pinctrl.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index d2bb7f441..71a12e8b0 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1666,6 +1666,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>           }
>   
>           struct icmp_header *ih = dp_packet_l4(&packet);
> +
> +        /* The packet's L4 data was allocated and will never be NULL, inform
> +         * the compiler about that.
> +         */
> +        ovs_assert(ih);
> +
>           packet_set_icmp(&packet, ICMP4_DST_UNREACH, icmp_code);
>   
>           /* Include original IP + data. */
>

On Tue, Feb 1, 2022 at 11:46 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> Acked-by: Mark Michelson <mmichels@redhat.com>

Thanks.   I applied this patch to the main branch and branch-21.12.

Do let me know if it needs to be backported further down.

Numan



>
> On 1/28/22 11:14, Dumitru Ceara wrote:
> > GCC reported an out of bounds array access when compiling with -O2:
> >    controller/pinctrl.c: In function ‘pinctrl_handle_icmp’:
> >    controller/pinctrl.c:1673:9: error: ‘memcpy’ offset [0, 19] is out of the bounds [0, 0] [-Werror=array-bounds]
> >     1673 |         memcpy(data, in_ip, in_ip_len);
> >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > GCC version is: 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC)
> >
> > This happens because the compiler cannot infer that in this specific
> > case dp_packet_l4(&packet) can never return NULL.
> >
> > pinctrl_compose_ipv4() calls eth_compose() which calls
> > dp_packet_set_l3() and always sets 'l4_ofs' to a value that's different
> > from UINT16_MAX.
> >
> > Help the compiler out by asserting that the L4 data is not NULL.
> >
> > Simplified code sample shared on the gcc-help mailing list:
> > https://gcc.gnu.org/pipermail/gcc-help/2022-January/141160.html
> >
> > Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > Tested-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > ---
> >   controller/pinctrl.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index d2bb7f441..71a12e8b0 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -1666,6 +1666,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >           }
> >
> >           struct icmp_header *ih = dp_packet_l4(&packet);
> > +
> > +        /* The packet's L4 data was allocated and will never be NULL, inform
> > +         * the compiler about that.
> > +         */
> > +        ovs_assert(ih);
> > +
> >           packet_set_icmp(&packet, ICMP4_DST_UNREACH, icmp_code);
> >
> >           /* Include original IP + data. */
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 2/2/22 02:15, Numan Siddique wrote:
> On Tue, Feb 1, 2022 at 11:46 AM Mark Michelson <mmichels@redhat.com> wrote:
>>
>> Acked-by: Mark Michelson <mmichels@redhat.com>
> 
> Thanks.   I applied this patch to the main branch and branch-21.12.

Thanks!

> 
> Do let me know if it needs to be backported further down.

No, it's not needed further down.  The warning popped up after a recent
change.

Regards,
Dumitru