On 06/12/2017 04:36 PM, Lance Richardson wrote:
----- Original Message ----- > From: "Greg Rose" <[email protected]> > To: "Lance Richardson" <[email protected]> > Cc: [email protected] > 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 <[email protected]> >> --- >> 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 <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
