Skip to content

Commit

Permalink
Fixes a use-after-move in CopyCompressedInterStoreTask
Browse files Browse the repository at this point in the history
There was a use-after-move in `CopyCompressedInterStoreTask`. The issue
was highlighted by a failing `Async.CopyCompressedInterStore` C++ test.

The issue is that when we do the `write_compressed_sync` it copies the
`KeySegmentPair` by value (but that only copies the pointers and not the
underlying segment). Then when writing to S3 we move the `Segment` out
of the `shared_ptr`.

This commit fixes this by explicitly doing a deep copy of the
`KeySegmentPair` before doing the `write_compressed_sync`. And after
this the test passes.

The proper fix will be the `KeySegmentPair` refactor.
  • Loading branch information
IvoDD committed Feb 3, 2025
1 parent ff22094 commit 14bb27f
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions cpp/arcticdb/async/tasks.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <arcticdb/storage/library.hpp>
#include <arcticdb/storage/storage_options.hpp>
#include <arcticdb/storage/store.hpp>
#include <arcticdb/storage/key_segment_pair.hpp>
#include <arcticdb/entity/types.hpp>
#include <arcticdb/util/hash.hpp>
#include <arcticdb/stream/stream_utils.hpp>
Expand Down Expand Up @@ -396,9 +397,18 @@ struct CopyCompressedInterStoreTask : async::BaseTask {
key_segment_pair.set_key(*key_to_write_);
}

for (auto & target_store : target_stores_) {
for (auto i=0u; i<target_stores_.size(); ++i) {
auto& target_store = target_stores_[i];
try {
target_store->write_compressed_sync(key_segment_pair);
auto key_segment_pair_copy = key_segment_pair;
if (i < target_stores_.size()-1) {
// If this is not the last target we want to do a deep copy because write_compressed_sync will
// move out of the underlying segment.
auto key_copy = key_segment_pair.variant_key();
auto segment_copy = key_segment_pair.segment().clone();
key_segment_pair_copy = storage::KeySegmentPair(std::move(key_copy), std::move(segment_copy));
}
target_store->write_compressed_sync(key_segment_pair_copy);
} catch (const storage::DuplicateKeyException& e) {
log::storage().debug("Key {} already exists on the target: {}", variant_key_view(key_to_read_), e.what());
} catch (const std::exception& e) {
Expand Down

0 comments on commit 14bb27f

Please sign in to comment.