From d04fca3b1921a9ad1c69264573545a52e7adf120 Mon Sep 17 00:00:00 2001 From: Michael Penick Date: Mon, 28 Mar 2016 15:30:32 -0700 Subject: [PATCH 1/2] Fix: Crash caused by tables with the same name in adjacent keyspaces --- src/metadata.cpp | 29 ++++++------ src/metadata.hpp | 18 ++++---- .../src/test_schema_metadata.cpp | 44 +++++++++++++++++++ 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/src/metadata.cpp b/src/metadata.cpp index 308b90188..41c482afd 100644 --- a/src/metadata.cpp +++ b/src/metadata.cpp @@ -1303,8 +1303,9 @@ const ColumnMetadata* TableMetadataBase::get_column(const std::string& name) con } void TableMetadataBase::add_column(const ColumnMetadata::Ptr& column) { - columns_.push_back(column); - columns_by_name_[column->name()] = column; + if (columns_by_name_.insert(std::make_pair(column->name(), column)).second) { + columns_.push_back(column); + } } void TableMetadataBase::clear_columns() { @@ -1522,8 +1523,9 @@ const IndexMetadata* TableMetadata::get_index(const std::string& name) const { } void TableMetadata::add_index(const IndexMetadata::Ptr& index) { - indexes_.push_back(index); - indexes_by_name_[index->name()] = index; + if (indexes_by_name_.insert(std::make_pair(index->name(), index)).second) { + indexes_.push_back(index); + } } void TableMetadata::clear_indexes() { @@ -2189,6 +2191,7 @@ void Metadata::InternalData::update_columns(const MetadataConfig& config, Result if (keyspace_name != temp_keyspace_name) { keyspace_name = temp_keyspace_name; keyspace = get_or_create_keyspace(keyspace_name); + table_or_view_name.clear(); } if (table_or_view_name != temp_table_or_view_name) { @@ -2231,11 +2234,11 @@ void Metadata::InternalData::update_legacy_indexes(const MetadataConfig& config, while (rows.next()) { std::string temp_keyspace_name; - std::string temp_table; + std::string temp_table_name; const Row* row = rows.row(); if (!row->get_string_by_name("keyspace_name", &temp_keyspace_name) || - !row->get_string_by_name(table_column_name(config.cassandra_version), &temp_table) || + !row->get_string_by_name(table_column_name(config.cassandra_version), &temp_table_name) || !row->get_string_by_name("column_name", &column_name)) { LOG_ERROR("Unable to get column value for 'keyspace_name', '%s' or 'column_name'", table_column_name(config.cassandra_version)); @@ -2245,14 +2248,11 @@ void Metadata::InternalData::update_legacy_indexes(const MetadataConfig& config, if (keyspace_name != temp_keyspace_name) { keyspace_name = temp_keyspace_name; keyspace = get_or_create_keyspace(keyspace_name); + table_name.clear(); } - if (table_name != temp_table) { - // Build keys for the previous table - if (table) { - table->build_keys_and_sort(config); - } - table_name = temp_table; + if (table_name != temp_table_name) { + table_name = temp_table_name; table = keyspace->get_table(table_name); if (!table) continue; table->clear_indexes(); @@ -2300,13 +2300,10 @@ void Metadata::InternalData::update_indexes(const MetadataConfig& config, Result if (keyspace_name != temp_keyspace_name) { keyspace_name = temp_keyspace_name; keyspace = get_or_create_keyspace(keyspace_name); + table_name.clear(); } if (table_name != temp_table_name) { - // Build keys for the previous table - if (table) { - table->build_keys_and_sort(config); - } table_name = temp_table_name; table = keyspace->get_table(table_name); if (!table) continue; diff --git a/src/metadata.hpp b/src/metadata.hpp index bc4c5885b..7320c1c28 100644 --- a/src/metadata.hpp +++ b/src/metadata.hpp @@ -49,7 +49,7 @@ class MapIteratorImpl { MapIteratorImpl(const Collection& map) : next_(map.begin()) - , end_(map.end()) {} + , end_(map.end()) { } bool next() { if (next_ == end_) { @@ -77,7 +77,7 @@ class VecIteratorImpl { VecIteratorImpl(const Collection& vec) : next_(vec.begin()) - , end_(vec.end()) {} + , end_(vec.end()) { } bool next() { if (next_ == end_) { @@ -109,17 +109,17 @@ class MetadataField { public: typedef std::map Map; - MetadataField() {} + MetadataField() { } MetadataField(const std::string& name) - : name_(name) {} + : name_(name) { } MetadataField(const std::string& name, const Value& value, const SharedRefPtr& buffer) : name_(name) , value_(value) - , buffer_(buffer) {} + , buffer_(buffer) { } const std::string& name() const { return name_; @@ -141,7 +141,7 @@ class MetadataFieldIterator : public Iterator { MetadataFieldIterator(const Map& map) : Iterator(CASS_ITERATOR_TYPE_META_FIELD) - , impl_(map) {} + , impl_(map) { } virtual bool next() { return impl_.next(); } const MetadataField* field() const { return &impl_.item(); } @@ -184,7 +184,7 @@ class MetadataIteratorImpl : public Iterator { MetadataIteratorImpl(CassIteratorType type, const Collection& colleciton) : Iterator(type) - , impl_(colleciton) {} + , impl_(colleciton) { } virtual bool next() { return impl_.next(); } @@ -275,7 +275,9 @@ class IndexMetadata : public MetadataBase, public RefCounted { const Value* options() const { return options_; } IndexMetadata(const std::string& index_name) - : MetadataBase(index_name) { } + : MetadataBase(index_name) + , type_(CASS_INDEX_TYPE_UNKNOWN) + , options_(NULL) { } static IndexMetadata::Ptr from_row(const std::string& index_name, const SharedRefPtr& buffer, const Row* row); diff --git a/test/integration_tests/src/test_schema_metadata.cpp b/test/integration_tests/src/test_schema_metadata.cpp index 27aeaf7a7..20889de1a 100644 --- a/test/integration_tests/src/test_schema_metadata.cpp +++ b/test/integration_tests/src/test_schema_metadata.cpp @@ -1720,4 +1720,48 @@ BOOST_AUTO_TEST_CASE(materialized_view_clustering_order) { } } +/** + * Test duplicate table name in mulitple keyspaces. + * + * Verifies the case where two tables have the same name and their keyspaces + * are adjacent. There was an issue where columns and indexes from the second + * table would be added to previous keyspace's table. + * + * @since 2.3.0 + * @jira_ticket CPP-348 + * @test_category schema + * @cassandra_version 1.2.x + */ +BOOST_AUTO_TEST_CASE(duplicate_table_name) { + test_utils::execute_query(session, "CREATE KEYSPACE test14 WITH replication = " + "{ 'class' : 'SimpleStrategy', 'replication_factor' : 3 }"); + + test_utils::execute_query(session, "CREATE TABLE test14.table1 (key1 TEXT PRIMARY KEY, value1 INT)" ); + + test_utils::execute_query(session, "CREATE INDEX index1 ON test14.table1 (value1)"); + + test_utils::execute_query(session, "CREATE KEYSPACE test15 WITH replication = " + "{ 'class' : 'SimpleStrategy', 'replication_factor' : 3 }"); + + test_utils::execute_query(session, "CREATE TABLE test15.table1 (key1 TEXT PRIMARY KEY, value1 INT)" ); + + test_utils::execute_query(session, "CREATE INDEX index1 ON test15.table1 (value1)"); + + create_session(); + + refresh_schema_meta(); + + { + const CassTableMeta* table_meta = schema_get_table("test14", "table1"); + BOOST_CHECK(cass_table_meta_column_by_name(table_meta, "key1") != NULL); + BOOST_CHECK(cass_table_meta_index_by_name(table_meta, "index1") != NULL); + } + + { + const CassTableMeta* table_meta = schema_get_table("test15", "table1"); + BOOST_CHECK(cass_table_meta_column_by_name(table_meta, "key1") != NULL); + BOOST_CHECK(cass_table_meta_index_by_name(table_meta, "index1") != NULL); + } +} + BOOST_AUTO_TEST_SUITE_END() From 06a122b99feb6b8f360bd5f2b428177229babd3b Mon Sep 17 00:00:00 2001 From: Michael Penick Date: Mon, 28 Mar 2016 16:51:40 -0700 Subject: [PATCH 2/2] Fix: Invalid memory read and test leak --- src/metadata.cpp | 19 +++------ src/metadata.hpp | 12 +++--- src/replication_strategy.cpp | 39 +++++++++++-------- .../src/test_schema_metadata.cpp | 1 + 4 files changed, 34 insertions(+), 37 deletions(-) diff --git a/src/metadata.cpp b/src/metadata.cpp index 41c482afd..c03416994 100644 --- a/src/metadata.cpp +++ b/src/metadata.cpp @@ -1189,8 +1189,8 @@ void KeyspaceMetadata::update(const MetadataConfig& config, const SharedRefPtrto_string_ref() == "class") { strategy_class_ = value->to_string_ref(); } - strategy_options_[key->to_string_ref()] = value->to_string_ref(); } + strategy_options_ = *map; } } else { const Value* value = add_field(buffer, row, "strategy_class"); @@ -1198,18 +1198,9 @@ void KeyspaceMetadata::update(const MetadataConfig& config, const SharedRefPtrvalue_type())) { strategy_class_ = value->to_string_ref(); } - const Value* map = add_json_map_field(config.protocol_version, row, "strategy_options"); - if (map != NULL && - map->value_type() == CASS_VALUE_TYPE_MAP && - is_string_type(map->primary_value_type()) && - is_string_type(map->secondary_value_type())) { - MapIterator iterator(map); - while (iterator.next()) { - const Value* key = iterator.key(); - const Value* value = iterator.value(); - strategy_options_[key->to_string_ref()] = value->to_string_ref(); - } + if (map != NULL) { + strategy_options_ = *map; } } } @@ -1720,7 +1711,7 @@ void IndexMetadata::update(StringRef kind, const Value* options) { } } - options_ = options; + options_ = *options; } IndexMetadata::Ptr IndexMetadata::from_legacy(const MetadataConfig& config, @@ -1746,7 +1737,7 @@ IndexMetadata::Ptr IndexMetadata::from_legacy(const MetadataConfig& config, void IndexMetadata::update_legacy(StringRef index_type, const ColumnMetadata* column, const Value* options) { type_ = index_type_from_string(index_type); target_ = target_from_legacy(column, options); - options_ = options; + options_ = *options; } std::string IndexMetadata::target_from_legacy(const ColumnMetadata* column, diff --git a/src/metadata.hpp b/src/metadata.hpp index 7320c1c28..f96487359 100644 --- a/src/metadata.hpp +++ b/src/metadata.hpp @@ -272,12 +272,11 @@ class IndexMetadata : public MetadataBase, public RefCounted { CassIndexType type() const { return type_; } const std::string& target() const { return target_; } - const Value* options() const { return options_; } + const Value* options() const { return &options_; } IndexMetadata(const std::string& index_name) : MetadataBase(index_name) - , type_(CASS_INDEX_TYPE_UNKNOWN) - , options_(NULL) { } + , type_(CASS_INDEX_TYPE_UNKNOWN) { } static IndexMetadata::Ptr from_row(const std::string& index_name, const SharedRefPtr& buffer, const Row* row); @@ -297,7 +296,7 @@ class IndexMetadata : public MetadataBase, public RefCounted { private: CassIndexType type_; std::string target_; - const Value* options_; + Value options_; private: DISALLOW_COPY_AND_ASSIGN(IndexMetadata); @@ -503,7 +502,6 @@ class KeyspaceMetadata : public MetadataBase { public: typedef std::map Map; typedef CopyOnWritePtr MapPtr; - typedef std::map OptionsMap; class TableIterator : public MetadataIteratorImpl > { public: @@ -575,11 +573,11 @@ class KeyspaceMetadata : public MetadataBase { void drop_aggregate(const std::string& full_aggregate_name); StringRef strategy_class() const { return strategy_class_; } - const OptionsMap& strategy_options() const { return strategy_options_; } + const Value* strategy_options() const { return &strategy_options_; } private: StringRef strategy_class_; - OptionsMap strategy_options_; + Value strategy_options_; CopyOnWritePtr tables_; CopyOnWritePtr views_; diff --git a/src/replication_strategy.cpp b/src/replication_strategy.cpp index fc38c593e..85b5f3953 100644 --- a/src/replication_strategy.cpp +++ b/src/replication_strategy.cpp @@ -27,25 +27,32 @@ namespace cass { -static void build_dc_replicas(const KeyspaceMetadata::OptionsMap& strategy_options, +static void build_dc_replicas(const KeyspaceMetadata& ks_meta, NetworkTopologyStrategy::DCReplicaCountMap* output) { - for (KeyspaceMetadata::OptionsMap::const_iterator i = strategy_options.begin(), - end = strategy_options.end(); - i != end; ++i) { - if (i->first != "class") { - size_t replication_factor = strtoul(i->second.to_string().c_str(), NULL, 10); - if (replication_factor > 0) { - (*output)[i->first.to_string()] = replication_factor; + const Value* strategy_options = ks_meta.strategy_options(); + if (strategy_options->is_map()) { + MapIterator iterator(strategy_options); + while (iterator.next()) { + if (iterator.key()->to_string_ref() != "class") { + size_t replication_factor = strtoul(iterator.value()->to_string().c_str(), NULL, 10); + if (replication_factor > 0) { + (*output)[iterator.key()->to_string()] = replication_factor; + } } } } } -static size_t get_replication_factor(const KeyspaceMetadata::OptionsMap& strategy_options) { +static size_t get_replication_factor(const KeyspaceMetadata& ks_meta) { size_t replication_factor = 0; - KeyspaceMetadata::OptionsMap::const_iterator i = strategy_options.find("replication_factor"); - if (i != strategy_options.end()) { - replication_factor = strtoul(i->second.to_string().c_str(), NULL, 10); + const Value* strategy_options = ks_meta.strategy_options(); + if (strategy_options->is_map()) { + MapIterator iterator(strategy_options); + while (iterator.next()) { + if (iterator.key()->to_string_ref() == "replication_factor") { + replication_factor = strtoul(iterator.value()->to_string().c_str(), NULL, 10); + } + } } if (replication_factor == 0) { LOG_WARN("Replication factor of 0"); @@ -60,11 +67,11 @@ SharedRefPtr ReplicationStrategy::from_keyspace_meta(const SharedRefPtr strategy; if (ends_with(strategy_class, NetworkTopologyStrategy::STRATEGY_CLASS)) { NetworkTopologyStrategy::DCReplicaCountMap replication_factors; - build_dc_replicas(ks_meta.strategy_options(), &replication_factors); + build_dc_replicas(ks_meta, &replication_factors); return SharedRefPtr(new NetworkTopologyStrategy(strategy_class.to_string(), replication_factors)); } else if (ends_with(strategy_class, SimpleStrategy::STRATEGY_CLASS)) { - size_t replication_factor = get_replication_factor(ks_meta.strategy_options()); + size_t replication_factor = get_replication_factor(ks_meta); return SharedRefPtr(new SimpleStrategy(strategy_class.to_string(), replication_factor)); } else { return SharedRefPtr(new NonReplicatedStrategy(strategy_class.to_string())); @@ -76,7 +83,7 @@ const std::string NetworkTopologyStrategy::STRATEGY_CLASS("NetworkTopologyStrate bool NetworkTopologyStrategy::equal(const KeyspaceMetadata& ks_meta) { if (ks_meta.strategy_class() != strategy_class_) return false; DCReplicaCountMap temp_rfs; - build_dc_replicas(ks_meta.strategy_options(), &temp_rfs); + build_dc_replicas(ks_meta, &temp_rfs); return replication_factors_ == temp_rfs; } @@ -161,7 +168,7 @@ const std::string SimpleStrategy::STRATEGY_CLASS("SimpleStrategy"); bool SimpleStrategy::equal(const KeyspaceMetadata& ks_meta) { if (ks_meta.strategy_class() != strategy_class_) return false; - return replication_factor_ == get_replication_factor(ks_meta.strategy_options()); + return replication_factor_ == get_replication_factor(ks_meta); } void SimpleStrategy::tokens_to_replicas(const TokenHostMap& primary, TokenReplicaMap* output) const { diff --git a/test/integration_tests/src/test_schema_metadata.cpp b/test/integration_tests/src/test_schema_metadata.cpp index 20889de1a..f250473a8 100644 --- a/test/integration_tests/src/test_schema_metadata.cpp +++ b/test/integration_tests/src/test_schema_metadata.cpp @@ -1747,6 +1747,7 @@ BOOST_AUTO_TEST_CASE(duplicate_table_name) { test_utils::execute_query(session, "CREATE INDEX index1 ON test15.table1 (value1)"); + close_session(); create_session(); refresh_schema_meta();