| Message ID | 20170527035923.14608-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
> On May 26, 2017, at 8:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > Code generated by this program includes constructs like this: > > switch (((uint64_t) vendor << 32) | (type << 16) | code) > > with variables uint32_t vendor, uint16_t type, uint16_t code. By C rules, > "type << 16" has type "int", which means that it will be sign-extended to > 64 bits when ORed with uint64_t. Thus, if 'type' has bit 15 set, then > the overall result will have all of its top 32 bits set, which is not > the desired result. > > This commit fixes the problem. > > No actual error types used in OVS or OpenFlow have bit 15 set, so this > does not fix a user-visible problem. > > Found by Coverity. > > Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762955&defectInstanceId=4304798&mergedDefectId=180406 > Signed-off-by: Ben Pfaff <blp@ovn.org> Acked-by: Justin Pettit <jpettit@ovn.org> --Justin
On Thu, Jun 01, 2017 at 05:02:59PM -0700, Justin Pettit wrote: > > > On May 26, 2017, at 8:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > Code generated by this program includes constructs like this: > > > > switch (((uint64_t) vendor << 32) | (type << 16) | code) > > > > with variables uint32_t vendor, uint16_t type, uint16_t code. By C rules, > > "type << 16" has type "int", which means that it will be sign-extended to > > 64 bits when ORed with uint64_t. Thus, if 'type' has bit 15 set, then > > the overall result will have all of its top 32 bits set, which is not > > the desired result. > > > > This commit fixes the problem. > > > > No actual error types used in OVS or OpenFlow have bit 15 set, so this > > does not fix a user-visible problem. > > > > Found by Coverity. > > > > Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762955&defectInstanceId=4304798&mergedDefectId=180406 > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Acked-by: Justin Pettit <jpettit@ovn.org> Thanks! I applied this to master and backported it as far as OVS 2.3.
diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors index 9642593d2458..6fea550f90b7 100755 --- a/build-aux/extract-ofp-errors +++ b/build-aux/extract-ofp-errors @@ -391,7 +391,7 @@ static const char *error_comments[OFPERR_N_ERRORS] = { static enum ofperr %s_decode(uint32_t vendor, uint16_t type, uint16_t code) { - switch (((uint64_t) vendor << 32) | (type << 16) | code) {""" % name) + switch (((uint64_t) vendor << 32) | (uint32_t) (type << 16) | code) {""" % name) found = set() for enum in names: if enum not in map: @@ -405,7 +405,7 @@ static enum ofperr vendor_s = "(%#xULL << 32) | " % vendor else: vendor_s = "" - print (" case %s(%d << 16) | %d:" % (vendor_s, type_, code)) + print (" case %s(uint32_t) (%d << 16) | %d:" % (vendor_s, type_, code)) print (" return OFPERR_%s;" % enum) print ("""\ }
Code generated by this program includes constructs like this: switch (((uint64_t) vendor << 32) | (type << 16) | code) with variables uint32_t vendor, uint16_t type, uint16_t code. By C rules, "type << 16" has type "int", which means that it will be sign-extended to 64 bits when ORed with uint64_t. Thus, if 'type' has bit 15 set, then the overall result will have all of its top 32 bits set, which is not the desired result. This commit fixes the problem. No actual error types used in OVS or OpenFlow have bit 15 set, so this does not fix a user-visible problem. Found by Coverity. Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762955&defectInstanceId=4304798&mergedDefectId=180406 Signed-off-by: Ben Pfaff <blp@ovn.org> --- build-aux/extract-ofp-errors | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)