Skip to content

348 - Segfault when preparing statement for table whose name exists in multiple keyspaces #277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 19, 2016
Merged

348 - Segfault when preparing statement for table whose name exists in multiple keyspaces #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d04fca3
Fix: Crash caused by tables with the same name in adjacent keyspaces
Mar 28, 2016
06a122b
Fix: Invalid memory read and test leak
Mar 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 18 additions & 30 deletions src/metadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,27 +1189,18 @@ void KeyspaceMetadata::update(const MetadataConfig& config, const SharedRefPtr<R
if (key->to_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");
if (value != NULL &&
is_string_type(value->value_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;
}
}
}
Expand Down Expand Up @@ -1303,8 +1294,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() {
Expand Down Expand Up @@ -1522,8 +1514,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() {
Expand Down Expand Up @@ -1718,7 +1711,7 @@ void IndexMetadata::update(StringRef kind, const Value* options) {
}
}

options_ = options;
options_ = *options;
}

IndexMetadata::Ptr IndexMetadata::from_legacy(const MetadataConfig& config,
Expand All @@ -1744,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,
Expand Down Expand Up @@ -2189,6 +2182,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) {
Expand Down Expand Up @@ -2231,11 +2225,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));
Expand All @@ -2245,14 +2239,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();
Expand Down Expand Up @@ -2300,13 +2291,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;
Expand Down
26 changes: 13 additions & 13 deletions src/metadata.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class MapIteratorImpl {

MapIteratorImpl(const Collection& map)
: next_(map.begin())
, end_(map.end()) {}
, end_(map.end()) { }

bool next() {
if (next_ == end_) {
Expand Down Expand Up @@ -77,7 +77,7 @@ class VecIteratorImpl {

VecIteratorImpl(const Collection& vec)
: next_(vec.begin())
, end_(vec.end()) {}
, end_(vec.end()) { }

bool next() {
if (next_ == end_) {
Expand Down Expand Up @@ -109,17 +109,17 @@ class MetadataField {
public:
typedef std::map<std::string, MetadataField> Map;

MetadataField() {}
MetadataField() { }

MetadataField(const std::string& name)
: name_(name) {}
: name_(name) { }

MetadataField(const std::string& name,
const Value& value,
const SharedRefPtr<RefBuffer>& buffer)
: name_(name)
, value_(value)
, buffer_(buffer) {}
, buffer_(buffer) { }

const std::string& name() const {
return name_;
Expand All @@ -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(); }
Expand Down Expand Up @@ -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(); }

Expand Down Expand Up @@ -272,10 +272,11 @@ class IndexMetadata : public MetadataBase, public RefCounted<IndexMetadata> {

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) { }
: MetadataBase(index_name)
, type_(CASS_INDEX_TYPE_UNKNOWN) { }

static IndexMetadata::Ptr from_row(const std::string& index_name,
const SharedRefPtr<RefBuffer>& buffer, const Row* row);
Expand All @@ -295,7 +296,7 @@ class IndexMetadata : public MetadataBase, public RefCounted<IndexMetadata> {
private:
CassIndexType type_;
std::string target_;
const Value* options_;
Value options_;

private:
DISALLOW_COPY_AND_ASSIGN(IndexMetadata);
Expand Down Expand Up @@ -501,7 +502,6 @@ class KeyspaceMetadata : public MetadataBase {
public:
typedef std::map<std::string, KeyspaceMetadata> Map;
typedef CopyOnWritePtr<KeyspaceMetadata::Map> MapPtr;
typedef std::map<StringRef, StringRef> OptionsMap;

class TableIterator : public MetadataIteratorImpl<MapIteratorImpl<TableMetadata::Ptr> > {
public:
Expand Down Expand Up @@ -573,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<TableMetadata::Map> tables_;
CopyOnWritePtr<ViewMetadata::Map> views_;
Expand Down
39 changes: 23 additions & 16 deletions src/replication_strategy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -60,11 +67,11 @@ SharedRefPtr<ReplicationStrategy> ReplicationStrategy::from_keyspace_meta(const
SharedRefPtr<ReplicationStrategy> 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<ReplicationStrategy>(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<ReplicationStrategy>(new SimpleStrategy(strategy_class.to_string(), replication_factor));
} else {
return SharedRefPtr<ReplicationStrategy>(new NonReplicatedStrategy(strategy_class.to_string()));
Expand All @@ -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;
}

Expand Down Expand Up @@ -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 {
Expand Down
45 changes: 45 additions & 0 deletions test/integration_tests/src/test_schema_metadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,4 +1720,49 @@ 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)");

close_session();
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()