You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/04/20 00:56:48 UTC
[GitHub] [datasketches-cpp] alexey-milovidov opened a new issue #210: `theta_update_sketch_base` is not exception safe.
alexey-milovidov opened a new issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210
theta_update_sketch_base_impl.hpp:
```
template<typename EN, typename EK, typename A>
void theta_update_sketch_base<EN, EK, A>::resize() {
...
lg_cur_size_ += factor; <-- the size is increased here
...
entries_ = allocator_.allocate(new_size); <-- an exception can be thrown here
...
}
While unwinding the stack, destructor is called:
```
template<typename EN, typename EK, typename A>
theta_update_sketch_base<EN, EK, A>::~theta_update_sketch_base()
{
...
const size_t size = 1 << lg_cur_size_; <-- wrong size is calculated
for (size_t i = 0; i < size; ++i) {
if (EK()(entries_[i]) != 0) entries_[i].~EN(); <-- memory corruption
}
allocator_.deallocate(entries_, size); <-- method is called with wrong argument
}
}
```
The issue has been found while we tried to integrate the library into [ClickHouse](https://clickhouse.tech/):
https://github.com/ClickHouse/ClickHouse/pull/23334
ClickHouse has powerful fuzzing infrastructure (you can read about it [here](https://clickhouse.tech/blog/en/2021/fuzzing-clickhouse/)).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] alexey-milovidov commented on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
alexey-milovidov commented on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-823624650
Maybe `EK()` can throw? But I did not check what is EK yet...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] alexey-milovidov edited a comment on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
alexey-milovidov edited a comment on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-822892267
Memory leak is also suspected here:
```
EN* old_entries = entries_; <-- old_entries are owned by no one
entries_ = allocator_.allocate(new_size); <-- if exception is thrown, memory is leaked
for (size_t i = 0; i < new_size; ++i) EK()(entries_[i]) = 0;
num_entries_ = 0;
for (size_t i = 0; i < old_size; ++i) {
const uint64_t key = EK()(old_entries[i]);
if (key != 0) {
insert(find(key).first, std::move(old_entries[i])); // consider a special insert with no comparison
old_entries[i].~EN();
}
}
allocator_.deallocate(old_entries, old_size);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] alexey-milovidov edited a comment on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
alexey-milovidov edited a comment on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-822892267
Memory leak is also suspected here:
```
EN* old_entries = entries_; <-- old_entries are owned by no one
entries_ = allocator_.allocate(new_size);
/// if exception is thrown below, memory is leaked
for (size_t i = 0; i < new_size; ++i) EK()(entries_[i]) = 0;
num_entries_ = 0;
for (size_t i = 0; i < old_size; ++i) {
const uint64_t key = EK()(old_entries[i]);
if (key != 0) {
insert(find(key).first, std::move(old_entries[i])); // consider a special insert with no comparison
old_entries[i].~EN();
}
}
allocator_.deallocate(old_entries, old_size);
```
Maybe this place is ok - if all code between allocation and deallocation guaranteed to not throw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-823607425
I think I have a fix for the first issue. Regarding the second, you are right, potentially, if ~EN() can throw. Not in theta sketch, but in tuple sketch, where EN includes user type T. Perhaps we should always wrap pointers in uniq_ptr, but that adds so much ugly code because of custom allocator and deleter.
Besides, if ~EN() can throw, we can leak in the destructor of this class as well. Should we have try{} around each destructor call?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-823626138
no EK() is a trivial operation: for theta sketch it is pass-through, for tuple sketch it returns EN.first (it is an std::pair<uint64_t, T>)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] alexey-milovidov commented on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
alexey-milovidov commented on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-822892267
Memory leak is also suspected here:
```
EN* old_entries = entries_; <-- old_entries are owned by no one
entries_ = allocator_.allocate(new_size);
for (size_t i = 0; i < new_size; ++i) EK()(entries_[i]) = 0;
num_entries_ = 0;
for (size_t i = 0; i < old_size; ++i) {
const uint64_t key = EK()(old_entries[i]);
if (key != 0) {
insert(find(key).first, std::move(old_entries[i])); // consider a special insert with no comparison
old_entries[i].~EN();
}
}
allocator_.deallocate(old_entries, old_size);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] alexey-milovidov commented on issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
alexey-milovidov commented on issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210#issuecomment-823624232
Throwing destructors should not be considered, it's ok to assume that destructors don't throw or std::terminate is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov closed issue #210: `theta_update_sketch_base` is not exception safe.
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov closed issue #210:
URL: https://github.com/apache/datasketches-cpp/issues/210
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org