mirrors/scylladb
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

data/cell.hh: avoid accidental copies of non-nothrow copiable ranges

Browse Source 
`cell::make_collection()` assumes that all ranges passed to it are
nothrow copyable and movable views. This is not guaranteed, is not
expressed in the interface and is not mentioned in the comments either.
The changes introduced by 0a453e5d3a to collection serialization, making
it use fragmented buffers, fell into this trap, as it passes
`bytes_ostream` to `cell::make_collection()`. `bytes_ostream`'s copy
constructor allocates and hence can throw, triggering an
`std::terminate()` inside `cell::make_collection()` as the latter is
noexcept.

To solve this issue, non-nothrow copyable and movable ranges are now
wrapped in a `fragment_range_view` to make them so.
`cell::make_collection()` already requires callers to keep alive the
range for the duration of the call, so this does not introduce any new
requirements to the callers. Additionally, to avoid any future
accidents, do not accept temporaries for the `data` parameter. We don't
ever want to move this param anyway, we will either have a trivially
copyable view, or a potentially heavy-weight range that we will create a
trivially copyable view of.
This commit is contained in:
Botond Dénes
2020-01-08 17:53:28 +02:00
parent b52b4d36a2
commit 3ec889816a
1 changed files with 34 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
data/cell.hh
View File

@@ -276,6 +276,24 @@ private:
imr::alloc::context_factory<last_chunk_context>> lsa_last_chunk_migrate_fn;
static thread_local imr::alloc::lsa_migrate_fn<external_chunk,
imr::alloc::context_factory<chunk_context>> lsa_chunk_migrate_fn;
template<typename FragmentRangeView>
GCC6_CONCEPT(
requires std::is_nothrow_move_constructible_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_copy_constructible_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_copy_assignable_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_move_assignable_v<std::decay_t<FragmentRangeView>>
)
static auto do_make_collection(FragmentRangeView&& data) noexcept {
return [data] (auto&& serializer, auto&& allocations) noexcept {
return serializer
.serialize(imr::set_flag<tags::collection>(),
imr::set_flag<tags::external_data>(data.size_bytes() > maximum_internal_storage_length))
.template serialize_as<tags::collection>(variable_value::write(data), allocations)
.done();
};
}
public:
/// Make a writer that copies a cell
///
@@ -291,18 +309,25 @@ public:
/// \arg data needs to remain valid as long as the writer is in use.
/// \returns imr::WriterAllocator for cell::structure.
template<typename FragmentRange, typename = std::enable_if_t<is_fragment_range_v<std::decay_t<FragmentRange>>>>
static auto make_collection(FragmentRange&& data) noexcept {
return [data] (auto&& serializer, auto&& allocations) noexcept {
return serializer
.serialize(imr::set_flag<tags::collection>(),
imr::set_flag<tags::external_data>(data.size_bytes() > maximum_internal_storage_length))
.template serialize_as<tags::collection>(variable_value::write(data), allocations)
.done();
};
static auto make_collection(const FragmentRange& data) noexcept {
// The writer (`do_make_collection()`) is noexcept so we have to make
// sure the data can be copied/moved without the risk of throwing
// exceptions. If `data` is nothrow copyable already, which is the
// case when `data` is a view, we forward it as-is. Otherwise we
// wrap `data` in a view and forward the view. We require the caller
// to keep `data` alive anyway.
if constexpr (std::is_nothrow_move_constructible_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_copy_constructible_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_copy_assignable_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_move_assignable_v<std::decay_t<FragmentRange>>) {
return do_make_collection(data);
} else {
return do_make_collection(fragment_range_view(data));
}
}
static auto make_collection(bytes_view data) noexcept {
return make_collection(single_fragment_range(data));
return do_make_collection(single_fragment_range(data));
}
/// Make a writer for a dead cell