bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

large_data_handler: Fix a use after destruction

Browse Source 
The path leading to the issue was:

The sstable name is allocated and passed to maybe_delete_large_data_entries by reference

   auto name = sst->get_filename();
   return large_data_handler.maybe_delete_large_data_entries(*sst->get_schema(), name, sst->data_size());

A future is created with a reference to it

  large_partitions = with_sem([&s, &filename, this] {
     return delete_large_data_entries(s, filename, db::system_keyspace::LARGE_PARTITIONS);
  });

The semaphore blocks.

The filename is destroyed.

delete_large_data_entries is called with a destroyed filename.

The reason this did not reproduce trivially in a debug build was that
the sstable itself was in the stack and the destructed value was read
as an internal value, and so asan had nothing to complain about.

Unfortunately we also had no tests that the entry in
system.large_rows was actually deleted.

This patch passes the name by value. It might create up to 3 copies of
it. If that is too inefficient it can probably be avoided with a
do_with in maybe_delete_large_data_entries.

Fixes #4335

Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
This commit is contained in:
Rafael Ávila de Espíndola
2019-03-15 09:44:18 -07:00
parent ddf795d2f9
commit e7749e7aee
4 changed files with 13 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
db/large_data_handler.cc
View File

@@ -102,7 +102,7 @@ future<> cql_table_large_data_handler::record_large_rows(const sstables::sstable
}
}
future<> cql_table_large_data_handler::delete_large_data_entries(const schema& s, const sstring& sstable_name, std::string_view large_table_name) const {
future<> cql_table_large_data_handler::delete_large_data_entries(const schema& s, sstring sstable_name, std::string_view large_table_name) const {
static const sstring req =
format("DELETE FROM system.{} WHERE keyspace_name = ? AND table_name = ? AND sstable_name = ?",
large_table_name);

20
db/large_data_handler.hh
View File

@@ -104,24 +104,24 @@ public:
return make_ready_future<>();
}
future<> maybe_delete_large_data_entries(const schema& s, const sstring& filename, uint64_t data_size) {
future<> maybe_delete_large_data_entries(const schema& s, sstring filename, uint64_t data_size) {
assert(!stopped());
future<> large_partitions = make_ready_future<>();
if (__builtin_expect(data_size > _partition_threshold_bytes, false)) {
large_partitions = with_sem([&s, &filename, this] {
return delete_large_data_entries(s, filename, db::system_keyspace::LARGE_PARTITIONS);
large_partitions = with_sem([&s, filename, this] () mutable {
return delete_large_data_entries(s, std::move(filename), db::system_keyspace::LARGE_PARTITIONS);
});
}
future<> large_rows = make_ready_future<>();
if (__builtin_expect(data_size > _row_threshold_bytes, false)) {
large_rows = with_sem([&s, &filename, this] {
return delete_large_data_entries(s, filename, db::system_keyspace::LARGE_ROWS);
large_rows = with_sem([&s, filename, this] () mutable {
return delete_large_data_entries(s, std::move(filename), db::system_keyspace::LARGE_ROWS);
});
}
future<> large_cells = make_ready_future<>();
if (__builtin_expect(data_size > _cell_threshold_bytes, false)) {
large_cells = with_sem([&s, &filename, this] {
return delete_large_data_entries(s, filename, db::system_keyspace::LARGE_CELLS);
large_cells = with_sem([&s, filename, this] () mutable {
return delete_large_data_entries(s, std::move(filename), db::system_keyspace::LARGE_CELLS);
});
}
return when_all(std::move(large_partitions), std::move(large_rows), std::move(large_cells)).discard_result();
@@ -133,7 +133,7 @@ protected:
virtual future<> record_large_cells(const sstables::sstable& sst, const sstables::key& partition_key,
const clustering_key_prefix* clustering_key, const column_definition& cdef, uint64_t cell_size) const = 0;
virtual future<> record_large_rows(const sstables::sstable& sst, const sstables::key& partition_key, const clustering_key_prefix* clustering_key, uint64_t row_size) const = 0;
virtual future<> delete_large_data_entries(const schema& s, const sstring& sstable_name, std::string_view large_table_name) const = 0;
virtual future<> delete_large_data_entries(const schema& s, sstring sstable_name, std::string_view large_table_name) const = 0;
virtual future<> record_large_partitions(const sstables::sstable& sst, const sstables::key& partition_key, uint64_t partition_size) const = 0;
};
@@ -144,7 +144,7 @@ public:
protected:
virtual future<> record_large_partitions(const sstables::sstable& sst, const sstables::key& partition_key, uint64_t partition_size) const override;
virtual future<> delete_large_data_entries(const schema& s, const sstring& sstable_name, std::string_view large_table_name) const override;
virtual future<> delete_large_data_entries(const schema& s, sstring sstable_name, std::string_view large_table_name) const override;
virtual future<> record_large_cells(const sstables::sstable& sst, const sstables::key& partition_key,
const clustering_key_prefix* clustering_key, const column_definition& cdef, uint64_t cell_size) const override;
virtual future<> record_large_rows(const sstables::sstable& sst, const sstables::key& partition_key, const clustering_key_prefix* clustering_key, uint64_t row_size) const override;
@@ -159,7 +159,7 @@ public:
return make_ready_future<>();
}
virtual future<> delete_large_data_entries(const schema& s, const sstring& sstable_name, std::string_view large_table_name) const override {
virtual future<> delete_large_data_entries(const schema& s, sstring sstable_name, std::string_view large_table_name) const override {
return make_ready_future<>();
}

2
sstables/sstables.cc
View File

@@ -3170,7 +3170,7 @@ static future<>
maybe_delete_large_data_entry(shared_sstable sst, db::large_data_handler& large_data_handler)
{
auto name = sst->get_filename();
return large_data_handler.maybe_delete_large_data_entries(*sst->get_schema(), name, sst->data_size());
return large_data_handler.maybe_delete_large_data_entries(*sst->get_schema(), std::move(name), sst->data_size());
}
static future<>

2
tests/sstable_3_x_test.cc
View File

@@ -5031,7 +5031,7 @@ struct large_row_handler : public db::large_data_handler {
return make_ready_future<>();
}
virtual future<> delete_large_data_entries(const schema& s, const sstring& sstable_name, std::string_view) const override {
virtual future<> delete_large_data_entries(const schema& s, sstring sstable_name, std::string_view) const override {
return make_ready_future<>();
}
};