Commit e739084e authored by Giuseppe Ottaviano's avatar Giuseppe Ottaviano Committed by Facebook GitHub Bot

Revert D22304614: Fix overflow in EliasFanoReader

Differential Revision:
D22304614 (https://github.com/facebook/folly/commit/cf31609549e6085b1d9be523e67cd62f05287d5e)

Original commit changeset: bf846730c594

fbshipit-source-id: c1982958588fb0bf1194c84de93d6ed1336ccdb5
parent d671caf2
...@@ -87,8 +87,8 @@ struct EliasFanoCompressedListBase { ...@@ -87,8 +87,8 @@ struct EliasFanoCompressedListBase {
Pointer upper = nullptr; Pointer upper = nullptr;
}; };
using EliasFanoCompressedList = EliasFanoCompressedListBase<const uint8_t*>; typedef EliasFanoCompressedListBase<const uint8_t*> EliasFanoCompressedList;
using MutableEliasFanoCompressedList = EliasFanoCompressedListBase<uint8_t*>; typedef EliasFanoCompressedListBase<uint8_t*> MutableEliasFanoCompressedList;
template < template <
class Value, class Value,
...@@ -102,12 +102,11 @@ struct EliasFanoEncoderV2 { ...@@ -102,12 +102,11 @@ struct EliasFanoEncoderV2 {
std::is_integral<Value>::value && std::is_unsigned<Value>::value, std::is_integral<Value>::value && std::is_unsigned<Value>::value,
"Value should be unsigned integral"); "Value should be unsigned integral");
using CompressedList = EliasFanoCompressedList; typedef EliasFanoCompressedList CompressedList;
using MutableCompressedList = MutableEliasFanoCompressedList; typedef MutableEliasFanoCompressedList MutableCompressedList;
using ValueType = Value;
using SkipValueType = SkipValue;
typedef Value ValueType;
typedef SkipValue SkipValueType;
struct Layout; struct Layout;
static constexpr size_t skipQuantum = kSkipQuantum; static constexpr size_t skipQuantum = kSkipQuantum;
...@@ -374,7 +373,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>, ...@@ -374,7 +373,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
SkipPointers<Encoder::skipQuantum>(list.skipPointers), SkipPointers<Encoder::skipQuantum>(list.skipPointers),
start_(list.upper), start_(list.upper),
size_(list.size), size_(list.size),
upperBound_(estimateUpperBound(list)) { upperBound_(8 * list.upperSizeBytes - size_) {
reset(); reset();
} }
...@@ -507,8 +506,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>, ...@@ -507,8 +506,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
skip -= cnt; skip -= cnt;
position_ += kBitsPerBlock - cnt; position_ += kBitsPerBlock - cnt;
outer_ += sizeof(block_t); outer_ += sizeof(block_t);
DCHECK_LT(outer_, (static_cast<size_t>(upperBound_) + size() + 7) / 8) DCHECK_LT(outer_, (static_cast<size_t>(upperBound_) + size()) / 8);
<< upperBound_ << " " << size() << " " << v;
block_ = folly::loadUnaligned<block_t>(start_ + outer_); block_ = folly::loadUnaligned<block_t>(start_ + outer_);
} }
...@@ -592,34 +590,22 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>, ...@@ -592,34 +590,22 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
} }
private: private:
using block_t = uint64_t;
// The size in bytes of the upper bits is limited by n + universe / 8,
// so a type that can hold either sizes or values is sufficient.
using OuterType = typename std::common_type<ValueType, SizeType>::type;
static ValueType estimateUpperBound(
const typename Encoder::CompressedList& list) {
size_t upperBound = 8 * list.upperSizeBytes - list.size;
// The bitvector is byte-aligned, so we may be overestimating the universe
// size. Make sure it fits in ValueType.
return static_cast<ValueType>(std::min<size_t>(
upperBound,
std::numeric_limits<ValueType>::max() >> list.numLowerBits));
}
FOLLY_ALWAYS_INLINE bool setValue(size_t inner) { FOLLY_ALWAYS_INLINE bool setValue(size_t inner) {
value_ = static_cast<ValueType>(8 * outer_ + inner - position_); value_ = static_cast<ValueType>(8 * outer_ + inner - position_);
return true; return true;
} }
// dest is a position in the bit vector, so SizeType may not be FOLLY_ALWAYS_INLINE void reposition(SizeType dest) {
// sufficient here.
FOLLY_ALWAYS_INLINE void reposition(size_t dest) {
outer_ = dest / 8; outer_ = dest / 8;
block_ = folly::loadUnaligned<block_t>(start_ + outer_); block_ = folly::loadUnaligned<block_t>(start_ + outer_);
block_ &= ~((block_t(1) << (dest % 8)) - 1); block_ &= ~((block_t(1) << (dest % 8)) - 1);
} }
using block_t = uint64_t;
// The size in bytes of the upper bits is limited by n + universe / 8,
// so a type that can hold either sizes or values is sufficient.
using OuterType = typename std::common_type<ValueType, SizeType>::type;
FOLLY_ALWAYS_INLINE void FOLLY_ALWAYS_INLINE void
getPreviousInfo(block_t& block, size_t& inner, OuterType& outer) const { getPreviousInfo(block_t& block, size_t& inner, OuterType& outer) const {
DCHECK_NE(position(), std::numeric_limits<SizeType>::max()); DCHECK_NE(position(), std::numeric_limits<SizeType>::max());
...@@ -661,8 +647,8 @@ template < ...@@ -661,8 +647,8 @@ template <
class SizeType = typename Encoder::SkipValueType> class SizeType = typename Encoder::SkipValueType>
class EliasFanoReader { class EliasFanoReader {
public: public:
using EncoderType = Encoder; typedef Encoder EncoderType;
using ValueType = typename Encoder::ValueType; typedef typename Encoder::ValueType ValueType;
explicit EliasFanoReader(const typename Encoder::CompressedList& list) explicit EliasFanoReader(const typename Encoder::CompressedList& list)
: upper_(list), lower_(list.lower), numLowerBits_(list.numLowerBits) { : upper_(list), lower_(list.lower), numLowerBits_(list.numLowerBits) {
...@@ -788,8 +774,7 @@ class EliasFanoReader { ...@@ -788,8 +774,7 @@ class EliasFanoReader {
return true; return true;
} }
// We might be in the middle of a run of equal values, reposition by // We might be in the middle of a run, iterate backwards to the beginning.
// iterating backwards to its first element.
auto valueLower = Instructions::bzhi(value_, numLowerBits_); auto valueLower = Instructions::bzhi(value_, numLowerBits_);
while (!upper_.isAtBeginningOfRun() && while (!upper_.isAtBeginningOfRun() &&
readLowerPart(position() - 1) == valueLower) { readLowerPart(position() - 1) == valueLower) {
......
...@@ -249,7 +249,6 @@ void testSkipTo(const std::vector<uint64_t>& data, const List& list) { ...@@ -249,7 +249,6 @@ void testSkipTo(const std::vector<uint64_t>& data, const List& list) {
testSkipTo<Reader, List>(data, list, steps); testSkipTo<Reader, List>(data, list, steps);
} }
testSkipTo<Reader, List>(data, list, std::numeric_limits<size_t>::max()); testSkipTo<Reader, List>(data, list, std::numeric_limits<size_t>::max());
{ {
// Skip to the first element. // Skip to the first element.
Reader reader(list); Reader reader(list);
...@@ -257,32 +256,29 @@ void testSkipTo(const std::vector<uint64_t>& data, const List& list) { ...@@ -257,32 +256,29 @@ void testSkipTo(const std::vector<uint64_t>& data, const List& list) {
EXPECT_EQ(reader.value(), data[0]); EXPECT_EQ(reader.value(), data[0]);
EXPECT_EQ(reader.position(), 0); EXPECT_EQ(reader.position(), 0);
} }
{
// Skip past the last element. // Skip past the last element.
Reader reader(list);
EXPECT_FALSE(reader.skipTo(data.back() + 1));
EXPECT_FALSE(reader.valid());
EXPECT_EQ(reader.position(), reader.size());
EXPECT_FALSE(reader.next());
}
{
// Skip to maximum integer.
Reader reader(list);
using ValueType = typename Reader::ValueType;
EXPECT_FALSE(reader.skipTo(std::numeric_limits<ValueType>::max()));
EXPECT_FALSE(reader.valid());
EXPECT_EQ(reader.position(), reader.size());
EXPECT_FALSE(reader.next());
}
// Skip past the last element and before the upperBound.
using ValueType = typename Reader::ValueType; using ValueType = typename Reader::ValueType;
std::vector<ValueType> valuesPastTheEnd = { if (const auto upperBound = getUniverseUpperBound<ValueType>(list);
// max() is not representable, so both values are past the end. upperBound && *upperBound != data.back()) {
static_cast<ValueType>(data.back() + 1),
std::numeric_limits<ValueType>::max(),
};
// Exercise skipping past the last element but before the inferred upper
// bound.
if (const auto upperBound =
getUniverseUpperBound<ValueType>(list).value_or(data.back() + 1);
upperBound != data.back()) {
ValueType base = data.back() + 1;
for (ValueType value = base + 1;
// Stop for overflow.
value > base && value < upperBound;
value += value - base) {
valuesPastTheEnd.push_back(value);
}
}
for (auto value : valuesPastTheEnd) {
Reader reader(list); Reader reader(list);
EXPECT_FALSE(reader.skipTo(value)); EXPECT_FALSE(reader.skipTo(*upperBound));
EXPECT_FALSE(reader.valid()); EXPECT_FALSE(reader.valid());
EXPECT_EQ(reader.position(), reader.size()); EXPECT_EQ(reader.position(), reader.size());
EXPECT_FALSE(reader.next()); EXPECT_FALSE(reader.next());
......
...@@ -34,16 +34,16 @@ namespace compression { ...@@ -34,16 +34,16 @@ namespace compression {
// Overload to help CodingTestUtils retrieve the universe upperbound // Overload to help CodingTestUtils retrieve the universe upperbound
// of the list for certain test cases. // of the list for certain test cases.
template <typename ValueType, typename T> template <typename ValueType, typename T>
folly::Optional<ValueType> getUniverseUpperBound( folly::Optional<std::size_t> getUniverseUpperBound(
const EliasFanoCompressedListBase<T>& list) { const EliasFanoCompressedListBase<T>& list) {
constexpr size_t kMaxUpperValue = std::numeric_limits<ValueType>::max(); constexpr ValueType maxUpperValue = std::numeric_limits<ValueType>::max();
const size_t maxUpperBits = kMaxUpperValue >> list.numLowerBits; const ValueType maxUpperBits = maxUpperValue >> list.numLowerBits;
const ValueType upperBitsUniverse = static_cast<ValueType>( const ValueType upperBitsUniverse = std::min(
std::min(8 * list.upperSizeBytes - list.size, maxUpperBits)); static_cast<ValueType>(8 * list.upperSizeBytes - list.size),
maxUpperBits);
return (upperBitsUniverse << list.numLowerBits) | return (upperBitsUniverse << list.numLowerBits) |
((ValueType(1) << list.numLowerBits) - 1); ((1 << list.numLowerBits) - 1);
} }
} // namespace compression } // namespace compression
} // namespace folly } // namespace folly
...@@ -134,7 +134,7 @@ class EliasFanoCodingTest : public ::testing::Test { ...@@ -134,7 +134,7 @@ class EliasFanoCodingTest : public ::testing::Test {
testAll<Reader, Encoder>(generateSeqList(1, 100000, 100)); testAll<Reader, Encoder>(generateSeqList(1, 100000, 100));
// max() cannot be read, as it is assumed an invalid value. // max() cannot be read, as it is assumed an invalid value.
// TODO(ott): It should be possible to lift this constraint. // TODO(ott): It should be possible to lift this constraint.
testAll<Reader, Encoder>({0, 1, std::numeric_limits<ValueType>::max() - 1}); testAll<Reader, Encoder>({0, 1, std::numeric_limits<uint32_t>::max() - 1});
// Test data with additional trailing 0s in the upperBits by extending // Test data with additional trailing 0s in the upperBits by extending
// the upper bound. // the upper bound.
constexpr uint64_t minUpperBoundExtension = 2; constexpr uint64_t minUpperBoundExtension = 2;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment