| Message ID | 20170613045114.1511-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
> From: "Ben Pfaff" <blp@ovn.org> > To: dev@openvswitch.org > Cc: "Ben Pfaff" <blp@ovn.org>, "Lance Richardson" <lrichard@redhat.com> > Sent: Tuesday, 13 June, 2017 12:51:14 AM > Subject: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > 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. > > Also document and convert a macro to a function. > > While we're at it, delete the unused macro BE16S_TO_BE32. > > Found via gcc's undefined behavior sanitizer. > > Reported-by: Lance Richardson <lrichard@redhat.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/byte-order.h | 21 +++++++++++++-------- > lib/flow.c | 2 +- > 2 files changed, 14 insertions(+), 9 deletions(-) > Looks good. Acked-by: Lance Richardson <lrichard@redhat.com>
On Tue, Jun 13, 2017 at 09:09:44AM -0400, Lance Richardson wrote: > > From: "Ben Pfaff" <blp@ovn.org> > > To: dev@openvswitch.org > > Cc: "Ben Pfaff" <blp@ovn.org>, "Lance Richardson" <lrichard@redhat.com> > > Sent: Tuesday, 13 June, 2017 12:51:14 AM > > Subject: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > > > 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. > > > > Also document and convert a macro to a function. > > > > While we're at it, delete the unused macro BE16S_TO_BE32. > > > > Found via gcc's undefined behavior sanitizer. > > > > Reported-by: Lance Richardson <lrichard@redhat.com> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > lib/byte-order.h | 21 +++++++++++++-------- > > lib/flow.c | 2 +- > > 2 files changed, 14 insertions(+), 9 deletions(-) > > > > Looks good. > > Acked-by: Lance Richardson <lrichard@redhat.com> Thanks. I applied this to master. Do you think that it is worthwhile to apply this to older branches?
> From: "Ben Pfaff" <blp@ovn.org> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: dev@openvswitch.org > Sent: Tuesday, 13 June, 2017 11:17:26 AM > Subject: Re: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > On Tue, Jun 13, 2017 at 09:09:44AM -0400, Lance Richardson wrote: > > > From: "Ben Pfaff" <blp@ovn.org> > > > To: dev@openvswitch.org > > > Cc: "Ben Pfaff" <blp@ovn.org>, "Lance Richardson" <lrichard@redhat.com> > > > Sent: Tuesday, 13 June, 2017 12:51:14 AM > > > Subject: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > > > > > 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. > > > > > > Also document and convert a macro to a function. > > > > > > While we're at it, delete the unused macro BE16S_TO_BE32. > > > > > > Found via gcc's undefined behavior sanitizer. > > > > > > Reported-by: Lance Richardson <lrichard@redhat.com> > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > lib/byte-order.h | 21 +++++++++++++-------- > > > lib/flow.c | 2 +- > > > 2 files changed, 14 insertions(+), 9 deletions(-) > > > > > > > Looks good. > > > > Acked-by: Lance Richardson <lrichard@redhat.com> > > Thanks. I applied this to master. > > Do you think that it is worthwhile to apply this to older branches? > I would guess not... the only danger here would be if the compiler incorrectly optimized something based on inferring that the high-order bit of the value being shifted 24 bits has to be zero because there would be "undefined behavior" if it were set. From looking through the code, there doesn't seem to be any real exposure. Thanks, Lance
On Tue, Jun 13, 2017 at 11:34:25AM -0400, Lance Richardson wrote: > > From: "Ben Pfaff" <blp@ovn.org> > > To: "Lance Richardson" <lrichard@redhat.com> > > Cc: dev@openvswitch.org > > Sent: Tuesday, 13 June, 2017 11:17:26 AM > > Subject: Re: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > > > On Tue, Jun 13, 2017 at 09:09:44AM -0400, Lance Richardson wrote: > > > > From: "Ben Pfaff" <blp@ovn.org> > > > > To: dev@openvswitch.org > > > > Cc: "Ben Pfaff" <blp@ovn.org>, "Lance Richardson" <lrichard@redhat.com> > > > > Sent: Tuesday, 13 June, 2017 12:51:14 AM > > > > Subject: [PATCH] byte-order: Fix undefined behavior of BYTES_TO_BE32. > > > > > > > > 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. > > > > > > > > Also document and convert a macro to a function. > > > > > > > > While we're at it, delete the unused macro BE16S_TO_BE32. > > > > > > > > Found via gcc's undefined behavior sanitizer. > > > > > > > > Reported-by: Lance Richardson <lrichard@redhat.com> > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > --- > > > > lib/byte-order.h | 21 +++++++++++++-------- > > > > lib/flow.c | 2 +- > > > > 2 files changed, 14 insertions(+), 9 deletions(-) > > > > > > > > > > Looks good. > > > > > > Acked-by: Lance Richardson <lrichard@redhat.com> > > > > Thanks. I applied this to master. > > > > Do you think that it is worthwhile to apply this to older branches? > > > > I would guess not... the only danger here would be if the compiler > incorrectly optimized something based on inferring that the high-order > bit of the value being shifted 24 bits has to be zero because there > would be "undefined behavior" if it were set. From looking through > the code, there doesn't seem to be any real exposure. OK, thanks.
diff --git a/lib/byte-order.h b/lib/byte-order.h index e864658f9b59..b2a9082bbada 100644 --- a/lib/byte-order.h +++ b/lib/byte-order.h @@ -98,17 +98,22 @@ uint32_byteswap(uint32_t crc) { ((((ovs_be64) (VALUE)) & UINT64_C(0xff00000000000000)) >> 56)) #endif +/* Returns the ovs_be32 that you would get from: + * + * union { uint8_t b[4]; ovs_be32 be32; } x = { .b = { b0, b1, b2, b3 } }; + * return x.be32; + * + * but without the undefined behavior. */ +static inline ovs_be32 +bytes_to_be32(uint8_t b0, uint8_t b1, uint8_t b2, uint8_t b3) +{ #if WORDS_BIGENDIAN -#define BYTES_TO_BE32(B1, B2, B3, B4) \ - (OVS_FORCE ovs_be32)((uint32_t)(B1) << 24 | (B2) << 16 | (B3) << 8 | (B4)) -#define BE16S_TO_BE32(B1, B2) \ - (OVS_FORCE ovs_be32)((uint32_t)(B1) << 16 | (B2)) + uint32_t x = ((uint32_t) b0 << 24) | (b1 << 16) | (b2 << 8) | b3; #else -#define BYTES_TO_BE32(B1, B2, B3, B4) \ - (OVS_FORCE ovs_be32)((uint32_t)(B1) | (B2) << 8 | (B3) << 16 | (B4) << 24) -#define BE16S_TO_BE32(B1, B2) \ - (OVS_FORCE ovs_be32)((uint32_t)(B1) | (B2) << 16) + uint32_t x = ((uint32_t) b3 << 24) | (b2 << 16) | (b1 << 8) | b0; #endif + return (OVS_FORCE ovs_be32) x; +} /* These functions zero-extend big-endian values to longer ones, * or truncate long big-endian value to shorter ones. */ diff --git a/lib/flow.c b/lib/flow.c index 1f51b66e7b44..d73e796a2f45 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -823,7 +823,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) packet->l4_ofs = (char *)data - frame; miniflow_push_be32(mf, nw_frag, - BYTES_TO_BE32(nw_frag, nw_tos, nw_ttl, nw_proto)); + bytes_to_be32(nw_frag, nw_tos, nw_ttl, nw_proto)); if (OVS_LIKELY(!(nw_frag & FLOW_NW_FRAG_LATER))) { if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
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. Also document and convert a macro to a function. While we're at it, delete the unused macro BE16S_TO_BE32. Found via gcc's undefined behavior sanitizer. Reported-by: Lance Richardson <lrichard@redhat.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/byte-order.h | 21 +++++++++++++-------- lib/flow.c | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-)