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

Lance Richardson lrichard at redhat.com
Mon Jun 12 23:36:48 UTC 2017



----- 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


More information about the dev mailing list