[ovs-dev] [PATCH] byte-order: avoid left shifts with unrepresentable results

Greg Rose gvrose8192 at gmail.com
Tue Jun 13 01:35:07 UTC 2017


On 06/12/2017 04:36 PM, Lance Richardson wrote:
>
>
> ----- Original Message -----
> > From: "Greg Rose" <gvrose8192 at gmail.com>
> > To: "Lance Richardson" <lrichard at redhat.com>
> > Cc: dev at openvswitch.org
> > Sent: Monday, 12 June, 2017 6:44:43 PM
> > Subject: Re: [ovs-dev] [PATCH] byte-order: avoid left shifts with unrepresentable results
> >
> > On 06/12/2017 01:13 PM, Lance Richardson wrote:
> >> A left shift that would produce a result that is not representable
> >> by the type of the expression's result has "undefined behavior"
> >> according to the C language standard. Avoid this by casting values
> >> that could set the upper bit to unsigned types.
> >>
> >> Found via gcc's undefined behavior sanitizer.
> >>
> >> Signed-off-by: Lance Richardson <lrichard at redhat.com>
> >> ---
> >>    lib/byte-order.h | 4 ++--
> >>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/byte-order.h b/lib/byte-order.h
> >> index e864658..782439f 100644
> >> --- a/lib/byte-order.h
> >> +++ b/lib/byte-order.h
> >> @@ -105,9 +105,9 @@ uint32_byteswap(uint32_t crc) {
> >>        (OVS_FORCE ovs_be32)((uint32_t)(B1) << 16 | (B2))
> >>    #else
> >>    #define BYTES_TO_BE32(B1, B2, B3, B4) \
> >> -    (OVS_FORCE ovs_be32)((uint32_t)(B1) | (B2) << 8 | (B3) << 16 | (B4) <<
> >> 24)
> >> +    (OVS_FORCE ovs_be32)((B1) | (B2) << 8 | (B3) << 16 | (uint32_t)(B4) <<
> >> 24)
> >
> > if B2 is a unsigned char then what is the value of this expression?
> > B2 << 8
>
> The more interesting question would be "what is the type of this expression?".
> It is "int" after integer promotions are done. The type of the expression
> "(B2) << 8 | (uint32_t)(B4)" is uint32_t. If B2 is an unsigned char, all possible
> values of B2 << 8 will fit.
>
> >
> > Same here.  If B3 is an unsigned char what is the value of this expression?
> > B3 << 16
>
> Ditto.
>
> >
> >>    #define BE16S_TO_BE32(B1, B2) \
> >> -    (OVS_FORCE ovs_be32)((uint32_t)(B1) | (B2) << 16)
> >> +    (OVS_FORCE ovs_be32)((B1) | (uint32_t)(B2) << 16)
> >>    #endif
> >>
> >>    /* These functions zero-extend big-endian values to longer ones,
> >>
> > I don't these macros.  There is no type checking so I think they could be
> > improved.
> >
> > I'd suggest turning them into inline functions so you get type checking, etc.
> >
>
> I tend to agree, but those benefits are orthogonal to the goal of this patch,
> which is simply to eliminate a case of undefined behavior that was detected
> while running OVS unit tests with the undefined behavior sanitizer enabled. A
> separate patch to convert these macros into inline functions would make sense,
> IMO.
>
> > My $0.02$
> >
> > Thanks,
> >
> > - Greg
> >
>
> Thanks,
>
>      Lance
>
OK, I'm convinced.

Thanks!!!

Reviewed-by: Greg Rose <gvrose8192 at gmail.com>



More information about the dev mailing list