You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by al...@apache.org on 2020/05/12 02:23:25 UTC
[incubator-datasketches-cpp] branch hll_exception_safety updated:
fixed leaks due to exceptions in deserialize
This is an automated email from the ASF dual-hosted git repository.
alsay pushed a commit to branch hll_exception_safety
in repository https://gitbox.apache.org/repos/asf/incubator-datasketches-cpp.git
The following commit(s) were added to refs/heads/hll_exception_safety by this push:
new bc8e191 fixed leaks due to exceptions in deserialize
bc8e191 is described below
commit bc8e19182e6fcc3ac126b9acc7f985d7145fab97
Author: AlexanderSaydakov <Al...@users.noreply.github.com>
AuthorDate: Mon May 11 19:23:13 2020 -0700
fixed leaks due to exceptions in deserialize
---
hll/include/AuxHashMap-internal.hpp | 8 ++--
hll/include/CouponHashSet-internal.hpp | 10 ++--
hll/include/CouponList-internal.hpp | 4 +-
hll/include/Hll4Array-internal.hpp | 2 +-
hll/include/HllArray-internal.hpp | 9 +++-
hll/include/HllSketch-internal.hpp | 4 ++
hll/test/HllSketchTest.cpp | 85 +++++++++++++++++++++++++++++++++-
7 files changed, 109 insertions(+), 13 deletions(-)
diff --git a/hll/include/AuxHashMap-internal.hpp b/hll/include/AuxHashMap-internal.hpp
index 6fc570f..9a8e135 100644
--- a/hll/include/AuxHashMap-internal.hpp
+++ b/hll/include/AuxHashMap-internal.hpp
@@ -118,8 +118,10 @@ AuxHashMap<A>* AuxHashMap<A>::deserialize(std::istream& is, const int lgConfigK,
lgArrInts = lgAuxArrInts;
}
- // TODO: truncated stream will throw exception without freeing memory
AuxHashMap<A>* auxHashMap = new (ahmAlloc().allocate(1)) AuxHashMap<A>(lgArrInts, lgConfigK);
+ typedef std::unique_ptr<AuxHashMap<A>, std::function<void(AuxHashMap<A>*)>> aux_hash_map_ptr;
+ aux_hash_map_ptr aux_ptr(auxHashMap, auxHashMap->make_deleter());
+
int configKmask = (1 << lgConfigK) - 1;
if (srcCompact) {
@@ -147,7 +149,7 @@ AuxHashMap<A>* AuxHashMap<A>::deserialize(std::istream& is, const int lgConfigK,
throw std::invalid_argument("Deserialized AuxHashMap has wrong number of entries");
}
- return auxHashMap;
+ return aux_ptr.release();
}
template<typename A>
@@ -159,7 +161,7 @@ AuxHashMap<A>::~AuxHashMap<A>() {
template<typename A>
std::function<void(AuxHashMap<A>*)> AuxHashMap<A>::make_deleter() {
- return [](AuxHashMap<A>* ptr) {
+ return [](AuxHashMap<A>* ptr) {
ptr->~AuxHashMap();
ahmAlloc().deallocate(ptr, 1);
};
diff --git a/hll/include/CouponHashSet-internal.hpp b/hll/include/CouponHashSet-internal.hpp
index 83a5adc..68415fe 100644
--- a/hll/include/CouponHashSet-internal.hpp
+++ b/hll/include/CouponHashSet-internal.hpp
@@ -169,6 +169,8 @@ CouponHashSet<A>* CouponHashSet<A>::newSet(std::istream& is) {
}
CouponHashSet<A>* sketch = new (chsAlloc().allocate(1)) CouponHashSet<A>(lgK, tgtHllType);
+ typedef std::unique_ptr<CouponHashSet<A>, std::function<void(HllSketchImpl<A>*)>> coupon_hash_set_ptr;
+ coupon_hash_set_ptr ptr(sketch, sketch->get_deleter());
// Don't set couponCount here;
// we'll set later if updatable, and increment with updates if compact
@@ -179,18 +181,16 @@ CouponHashSet<A>* CouponHashSet<A>::newSet(std::istream& is) {
sketch->couponUpdate(coupon);
}
} else {
- int* oldArr = sketch->couponIntArr;
- const size_t oldArrLen = 1 << sketch->lgCouponArrInts;
- sketch->lgCouponArrInts = lgArrInts;
typedef typename std::allocator_traits<A>::template rebind_alloc<int> intAlloc;
+ intAlloc().deallocate(sketch->couponIntArr, 1 << sketch->lgCouponArrInts);
+ sketch->lgCouponArrInts = lgArrInts;
sketch->couponIntArr = intAlloc().allocate(1 << lgArrInts);
sketch->couponCount = couponCount;
// for stream processing, read entire list so read pointer ends up set correctly
is.read((char*)sketch->couponIntArr, (1 << sketch->lgCouponArrInts) * sizeof(int));
- intAlloc().deallocate(oldArr, oldArrLen);
}
- return sketch;
+ return ptr.release();
}
template<typename A>
diff --git a/hll/include/CouponList-internal.hpp b/hll/include/CouponList-internal.hpp
index 83c9ee4..4e9c55a 100644
--- a/hll/include/CouponList-internal.hpp
+++ b/hll/include/CouponList-internal.hpp
@@ -173,6 +173,8 @@ CouponList<A>* CouponList<A>::newList(std::istream& is) {
const bool emptyFlag = ((listHeader[HllUtil<A>::FLAGS_BYTE] & HllUtil<A>::EMPTY_FLAG_MASK) ? true : false);
CouponList<A>* sketch = new (clAlloc().allocate(1)) CouponList<A>(lgK, tgtHllType, mode);
+ typedef std::unique_ptr<CouponList<A>, std::function<void(HllSketchImpl<A>*)>> coupon_list_ptr;
+ coupon_list_ptr ptr(sketch, sketch->get_deleter());
const int couponCount = listHeader[HllUtil<A>::LIST_COUNT_BYTE];
sketch->couponCount = couponCount;
sketch->putOutOfOrderFlag(oooFlag); // should always be false for LIST
@@ -185,7 +187,7 @@ CouponList<A>* CouponList<A>::newList(std::istream& is) {
is.read((char*)sketch->couponIntArr, numToRead * sizeof(int));
}
- return sketch;
+ return ptr.release();
}
template<typename A>
diff --git a/hll/include/Hll4Array-internal.hpp b/hll/include/Hll4Array-internal.hpp
index 30b24f7..8498bb8 100644
--- a/hll/include/Hll4Array-internal.hpp
+++ b/hll/include/Hll4Array-internal.hpp
@@ -175,7 +175,7 @@ void Hll4Array<A>::internalHll4Update(const int slotNo, const int newVal) {
//assert(shiftedNewValue >= 0);
if (rawStoredOldValue == HllUtil<A>::AUX_TOKEN) { // 879
- // Given that we have an AUX_TOKEN, tehre are 4 cases for how to
+ // Given that we have an AUX_TOKEN, there are 4 cases for how to
// actually modify the data structure
if (shiftedNewValue >= HllUtil<A>::AUX_TOKEN) { // case 1: 881
diff --git a/hll/include/HllArray-internal.hpp b/hll/include/HllArray-internal.hpp
index d5e2285..ac94a11 100644
--- a/hll/include/HllArray-internal.hpp
+++ b/hll/include/HllArray-internal.hpp
@@ -137,11 +137,14 @@ HllArray<A>* HllArray<A>::newHll(const void* bytes, size_t len) {
std::memcpy(&auxCount, data + HllUtil<A>::AUX_COUNT_INT, sizeof(int));
AuxHashMap<A>* auxHashMap = nullptr;
+ typedef std::unique_ptr<AuxHashMap<A>, std::function<void(AuxHashMap<A>*)>> aux_hash_map_ptr;
+ aux_hash_map_ptr aux_ptr;
if (auxCount > 0) { // necessarily TgtHllType == HLL_4
int auxLgIntArrSize = (int) data[4];
const size_t offset = HllUtil<A>::HLL_BYTE_ARR_START + arrayBytes;
const uint8_t* auxDataStart = data + offset;
auxHashMap = AuxHashMap<A>::deserialize(auxDataStart, len - offset, lgK, auxCount, auxLgIntArrSize, comapctFlag);
+ aux_ptr = aux_hash_map_ptr(auxHashMap, auxHashMap->make_deleter());
}
HllArray<A>* sketch = HllSketchImplFactory<A>::newHll(lgK, tgtHllType, startFullSizeFlag);
@@ -157,6 +160,7 @@ HllArray<A>* HllArray<A>::newHll(const void* bytes, size_t len) {
if (auxHashMap != nullptr)
((Hll4Array<A>*)sketch)->putAuxHashMap(auxHashMap);
+ aux_ptr.release();
return sketch;
}
@@ -188,8 +192,9 @@ HllArray<A>* HllArray<A>::newHll(std::istream& is) {
const int lgK = (int) listHeader[HllUtil<A>::LG_K_BYTE];
const int curMin = (int) listHeader[HllUtil<A>::HLL_CUR_MIN_BYTE];
- // TODO: truncated stream will throw exception without freeing memory
HllArray* sketch = HllSketchImplFactory<A>::newHll(lgK, tgtHllType, startFullSizeFlag);
+ typedef std::unique_ptr<HllArray<A>, std::function<void(HllSketchImpl<A>*)>> hll_array_ptr;
+ hll_array_ptr sketch_ptr(sketch, sketch->get_deleter());
sketch->putCurMin(curMin);
sketch->putOutOfOrderFlag(oooFlag);
@@ -214,7 +219,7 @@ HllArray<A>* HllArray<A>::newHll(std::istream& is) {
((Hll4Array<A>*)sketch)->putAuxHashMap(auxHashMap);
}
- return sketch;
+ return sketch_ptr.release();
}
template<typename A>
diff --git a/hll/include/HllSketch-internal.hpp b/hll/include/HllSketch-internal.hpp
index f05852e..0043b4d 100644
--- a/hll/include/HllSketch-internal.hpp
+++ b/hll/include/HllSketch-internal.hpp
@@ -280,6 +280,10 @@ std::ostream& hll_sketch_alloc<A>::to_string(std::ostream& os,
<< " HipAccum : " << hllArray->getHipAccum() << std::endl
<< " KxQ0 : " << hllArray->getKxQ0() << std::endl
<< " KxQ1 : " << hllArray->getKxQ1() << std::endl;
+ if (get_target_type() == HLL_4) {
+ const Hll4Array<A>* hll4_ptr = static_cast<const Hll4Array<A>*>(sketch_impl);
+ os << " Aux table? : " << (hll4_ptr->getAuxHashMap() != nullptr ? "true" : "false") << std::endl;
+ }
} else {
os << " Coupon count : "
<< std::to_string(((CouponList<A>*) sketch_impl)->getCouponCount()) << std::endl;
diff --git a/hll/test/HllSketchTest.cpp b/hll/test/HllSketchTest.cpp
index 622e89e..5691c78 100644
--- a/hll/test/HllSketchTest.cpp
+++ b/hll/test/HllSketchTest.cpp
@@ -291,7 +291,7 @@ TEST_CASE("hll sketch: check input types", "[hll_sketch]") {
REQUIRE(test_allocator_total_bytes == 0);
}
-TEST_CASE("hll sketch: deserialize buffer overrun", "[hll_sketch]") {
+TEST_CASE("hll sketch: deserialize list mode buffer overrun", "[hll_sketch]") {
test_allocator_total_bytes = 0;
{
hll_sketch_test_alloc sketch(10);
@@ -299,6 +299,89 @@ TEST_CASE("hll sketch: deserialize buffer overrun", "[hll_sketch]") {
auto bytes = sketch.serialize_compact();
REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), 7), std::out_of_range);
REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), bytes.size() - 1), std::out_of_range);
+
+ // ckeck for leaks on stream exceptions
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), 7));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), bytes.size() - 1));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ }
+ REQUIRE(test_allocator_total_bytes == 0);
+}
+
+TEST_CASE("hll sketch: deserialize set mode buffer overrun", "[hll_sketch]") {
+ test_allocator_total_bytes = 0;
+ {
+ hll_sketch_test_alloc sketch(10);
+ for (int i = 0; i < 10; ++i) sketch.update(i);
+ //sketch.to_string(std::cout);
+ auto bytes = sketch.serialize_updatable();
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), 7), std::out_of_range);
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), bytes.size() - 1), std::out_of_range);
+
+ // ckeck for leaks on stream exceptions
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), 7));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), bytes.size() - 1));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ }
+ REQUIRE(test_allocator_total_bytes == 0);
+}
+
+TEST_CASE("hll sketch: deserialize HLL mode buffer overrun", "[hll_sketch]") {
+ test_allocator_total_bytes = 0;
+ {
+ // this sketch should have aux table
+ hll_sketch_test_alloc sketch(15);
+ for (int i = 0; i < 14444; ++i) sketch.update(i);
+ //sketch.to_string(std::cout);
+ auto bytes = sketch.serialize_compact();
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), 7), std::out_of_range);
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), 15), std::out_of_range);
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), 16420), std::out_of_range); // before aux table
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(bytes.data(), bytes.size() - 1), std::out_of_range);
+
+ // ckeck for leaks on stream exceptions
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), 7));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), 15));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), 16420)); // before aux table
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
+ {
+ std::stringstream ss;
+ ss.exceptions(std::ios::failbit | std::ios::badbit);
+ ss.str(std::string((char*)bytes.data(), bytes.size() - 1));
+ REQUIRE_THROWS_AS(hll_sketch_test_alloc::deserialize(ss), std::ios_base::failure);
+ }
}
REQUIRE(test_allocator_total_bytes == 0);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org