Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6637
to review the following change.
Change subject: WIP: Add unsigned-integer overflow checking to UBSAN
......................................................................
WIP: Add unsigned-integer overflow checking to UBSAN
WIP because I haven't fixed any of these issues, only added
ignore annotations (a handful of them had to be fixed because they were
inside macro calls which can't be annotated).
Background:
We recently found a very subtle bug caused by unsigned integer overflow
in tablet_peer_mm_ops.cc. This patch was able to repro that overflow
from at least two different unit tests, so the value is high.
Additionally, it lets us be a little more aggressive about using
unsigned integers. I've never been much of a fan of the style guide's
ban.
Prelim Analysis:
A bunch of the overflow instances are expected, like inner loops in hash
functions. A bunch of the remaining issues are related to control flow
constructs, some of which look benign, some of which look like a bug.
Finally, it appears there are a few real issues in here, including the
bug which originally motivated this.
Next steps:
We need to go through each of these and either fix them, or justify
leaving the annotation. In the case of hash functions, leaving the
annotation is self-evident (especially since they are imported code),
but I'm hopeful that most of the others can be fixed. We may want to add
some annotated helper methods like overflowing_[add, subtract, multiply]
if there are a bunch of cases where overflow is wanted. This has the
upside of making the code more self-describing.
Change-Id: Ib0712f5307dcbab7fdffbe2364e1fdb691d3aaab
---
M CMakeLists.txt
A build-support/ubsan-blacklist.txt
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/index_btree.cc
M src/kudu/common/key_util.cc
M src/kudu/common/partition.cc
M src/kudu/gutil/hash/city.cc
M src/kudu/gutil/hash/hash128to64.h
M src/kudu/gutil/hash/jenkins.cc
M src/kudu/gutil/hash/jenkins_lookup2.h
M src/kudu/gutil/spinlock_internal.cc
M src/kudu/gutil/stl_util.h
M src/kudu/gutil/strings/fastmem.h
M src/kudu/gutil/strings/numbers.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/hybrid_clock-test.cc
M src/kudu/server/hybrid_clock.cc
M src/kudu/server/logical_clock.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_peer_mm_ops.cc
M src/kudu/util/alignment.h
M src/kudu/util/bit-stream-utils.inline.h
M src/kudu/util/bitmap-test.cc
M src/kudu/util/bloom_filter.h
M src/kudu/util/debug-util.cc
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/hash_util.h
M src/kudu/util/memory/overwrite.cc
M src/kudu/util/random_util.cc
M src/kudu/util/rle-encoding.h
M src/kudu/util/stopwatch.h
M src/kudu/util/thread.cc
33 files changed, 75 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6637/1
--
To view, visit http://gerrit.cloudera.org:8080/6637
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib0712f5307dcbab7fdffbe2364e1fdb691d3aaab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Todd Lipcon <[email protected]>
