| Message ID | 20170612201328.473-1-lrichard@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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@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 Same here. If B3 is an unsigned char what is the value of this expression? B3 << 16 > #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. My $0.02$ Thanks, - Greg
----- Original Message ----- > From: "Greg Rose" <gvrose8192@gmail.com> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: dev@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@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
On 06/12/2017 04:36 PM, Lance Richardson wrote: > > > ----- Original Message ----- > > From: "Greg Rose" <gvrose8192@gmail.com> > > To: "Lance Richardson" <lrichard@redhat.com> > > Cc: dev@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@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@gmail.com>
On Mon, Jun 12, 2017 at 07:36:48PM -0400, Lance Richardson wrote: > > > ----- Original Message ----- > > From: "Greg Rose" <gvrose8192@gmail.com> > > To: "Lance Richardson" <lrichard@redhat.com> > > Cc: dev@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@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. I don't see much downside to fixing all this in one go: https://patchwork.ozlabs.org/patch/774942/
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) #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,
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@redhat.com> --- lib/byte-order.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)