From 48e9d675257207e4edb9942756bec00600d5539c Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Feb 2016 12:23:09 +0100 Subject: [PATCH 1/4] compound: Extract size_type alias --- compound.hh | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/compound.hh b/compound.hh index da3b7eeebf..e702a269de 100644 --- a/compound.hh +++ b/compound.hh @@ -43,6 +43,7 @@ public: static constexpr bool is_prefixable = AllowPrefixes == allow_prefixes::yes; using prefix_type = compound_type; using value_type = std::vector; + using size_type = uint16_t; compound_type(std::vector types) : _types(std::move(types)) @@ -75,8 +76,8 @@ private: template static void serialize_value(RangeOfSerializedComponents&& values, bytes::iterator& out) { for (auto&& val : values) { - assert(val.size() <= std::numeric_limits::max()); - write(out, uint16_t(val.size())); + assert(val.size() <= std::numeric_limits::max()); + write(out, size_type(val.size())); out = std::copy(val.begin(), val.end(), out); } } @@ -84,7 +85,7 @@ private: static size_t serialized_size(RangeOfSerializedComponents&& values) { size_t len = 0; for (auto&& val : values) { - len += sizeof(uint16_t) + val.size(); + len += sizeof(size_type) + val.size(); } return len; } @@ -95,8 +96,8 @@ public: template static bytes serialize_value(RangeOfSerializedComponents&& values) { auto size = serialized_size(values); - if (size > std::numeric_limits::max()) { - throw std::runtime_error(sprint("Key size too large: %d > %d", size, std::numeric_limits::max())); + if (size > std::numeric_limits::max()) { + throw std::runtime_error(sprint("Key size too large: %d > %d", size, std::numeric_limits::max())); } bytes b(bytes::initialized_later(), size); auto i = b.begin(); @@ -135,13 +136,13 @@ public: value_type _current; private: void read_current() { - uint16_t len; + size_type len; { if (_v.empty()) { _v = bytes_view(nullptr, 0); return; } - len = read_simple(_v); + len = read_simple(_v); if (_v.size() < len) { throw marshal_exception(); } From 9375b7df7be390d4a46062c040495dff2a6b3a20 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Feb 2016 12:25:07 +0100 Subject: [PATCH 2/4] transport: server: Size check should allow max value --- transport/server.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/transport/server.cc b/transport/server.cc index d877f99bd1..29730140b7 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -1366,14 +1366,14 @@ void cql_server::response::write_short(int16_t n) void cql_server::response::write_string(const sstring& s) { - assert(s.size() < std::numeric_limits::max()); + assert(s.size() <= std::numeric_limits::max()); write_short(s.size()); _body.insert(_body.end(), s.begin(), s.end()); } void cql_server::response::write_long_string(const sstring& s) { - assert(s.size() < std::numeric_limits::max()); + assert(s.size() <= std::numeric_limits::max()); write_int(s.size()); _body.insert(_body.end(), s.begin(), s.end()); } @@ -1386,7 +1386,7 @@ void cql_server::response::write_uuid(utils::UUID uuid) void cql_server::response::write_string_list(std::vector string_list) { - assert(string_list.size() < std::numeric_limits::max()); + assert(string_list.size() <= std::numeric_limits::max()); write_short(string_list.size()); for (auto&& s : string_list) { write_string(s); @@ -1395,14 +1395,14 @@ void cql_server::response::write_string_list(std::vector string_list) void cql_server::response::write_bytes(bytes b) { - assert(b.size() < std::numeric_limits::max()); + assert(b.size() <= std::numeric_limits::max()); write_int(b.size()); _body.insert(_body.end(), b.begin(), b.end()); } void cql_server::response::write_short_bytes(bytes b) { - assert(b.size() < std::numeric_limits::max()); + assert(b.size() <= std::numeric_limits::max()); write_short(b.size()); _body.insert(_body.end(), b.begin(), b.end()); } @@ -1436,7 +1436,7 @@ void cql_server::response::write_consistency(db::consistency_level c) void cql_server::response::write_string_map(std::map string_map) { - assert(string_map.size() < std::numeric_limits::max()); + assert(string_map.size() <= std::numeric_limits::max()); write_short(string_map.size()); for (auto&& s : string_map) { write_string(s.first); @@ -1450,7 +1450,7 @@ void cql_server::response::write_string_multimap(std::multimap for (auto it = string_map.begin(), end = string_map.end(); it != end; it = string_map.upper_bound(it->first)) { keys.push_back(it->first); } - assert(keys.size() < std::numeric_limits::max()); + assert(keys.size() <= std::numeric_limits::max()); write_short(keys.size()); for (auto&& key : keys) { std::vector values; From 0ff0c5555a30dbe743bbbd0b79754977aaa77711 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Feb 2016 12:25:29 +0100 Subject: [PATCH 3/4] transport: server: 'short' should be unsigned According to CQL binary protocol v3 [1], "short" fields are unsigned: [short] A 2 bytes unsigned integer [1] https://git-wip-us.apache.org/repos/asf?p=cassandra.git;a=blob_plain;f=doc/native_protocol_v3.spec C* code agrees as well. Fixes #807. --- transport/server.cc | 18 +++++++++--------- transport/server.hh | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/transport/server.cc b/transport/server.cc index 29730140b7..d54accb63c 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -191,7 +191,7 @@ public: void write_byte(uint8_t b); void write_int(int32_t n); void write_long(int64_t n); - void write_short(int16_t n); + void write_short(uint16_t n); void write_string(const sstring& s); void write_long_string(const sstring& s); void write_uuid(utils::UUID uuid); @@ -1048,9 +1048,9 @@ int64_t cql_server::connection::read_long(bytes_view& buf) return n; } -int16_t cql_server::connection::read_short(bytes_view& buf) +uint16_t cql_server::connection::read_short(bytes_view& buf) { - return static_cast(read_unsigned_short(buf)); + return read_unsigned_short(buf); } uint16_t cql_server::connection::read_unsigned_short(bytes_view& buf) @@ -1357,7 +1357,7 @@ void cql_server::response::write_long(int64_t n) _body.insert(_body.end(), s, s+sizeof(u)); } -void cql_server::response::write_short(int16_t n) +void cql_server::response::write_short(uint16_t n) { auto u = htons(n); auto *s = reinterpret_cast(&u); @@ -1366,7 +1366,7 @@ void cql_server::response::write_short(int16_t n) void cql_server::response::write_string(const sstring& s) { - assert(s.size() <= std::numeric_limits::max()); + assert(s.size() <= std::numeric_limits::max()); write_short(s.size()); _body.insert(_body.end(), s.begin(), s.end()); } @@ -1386,7 +1386,7 @@ void cql_server::response::write_uuid(utils::UUID uuid) void cql_server::response::write_string_list(std::vector string_list) { - assert(string_list.size() <= std::numeric_limits::max()); + assert(string_list.size() <= std::numeric_limits::max()); write_short(string_list.size()); for (auto&& s : string_list) { write_string(s); @@ -1402,7 +1402,7 @@ void cql_server::response::write_bytes(bytes b) void cql_server::response::write_short_bytes(bytes b) { - assert(b.size() <= std::numeric_limits::max()); + assert(b.size() <= std::numeric_limits::max()); write_short(b.size()); _body.insert(_body.end(), b.begin(), b.end()); } @@ -1436,7 +1436,7 @@ void cql_server::response::write_consistency(db::consistency_level c) void cql_server::response::write_string_map(std::map string_map) { - assert(string_map.size() <= std::numeric_limits::max()); + assert(string_map.size() <= std::numeric_limits::max()); write_short(string_map.size()); for (auto&& s : string_map) { write_string(s.first); @@ -1450,7 +1450,7 @@ void cql_server::response::write_string_multimap(std::multimap for (auto it = string_map.begin(), end = string_map.end(); it != end; it = string_map.upper_bound(it->first)) { keys.push_back(it->first); } - assert(keys.size() <= std::numeric_limits::max()); + assert(keys.size() <= std::numeric_limits::max()); write_short(keys.size()); for (auto&& key : keys) { std::vector values; diff --git a/transport/server.hh b/transport/server.hh index 1f1d4c2097..a68e01eb23 100644 --- a/transport/server.hh +++ b/transport/server.hh @@ -178,7 +178,7 @@ private: int8_t read_byte(bytes_view& buf); int32_t read_int(bytes_view& buf); int64_t read_long(bytes_view& buf); - int16_t read_short(bytes_view& buf); + uint16_t read_short(bytes_view& buf); uint16_t read_unsigned_short(bytes_view& buf); sstring read_string(bytes_view& buf); sstring_view read_string_view(bytes_view& buf); From 6e7bac14b3a784ca6dead5ef2f002354b6b52356 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Feb 2016 12:59:40 +0100 Subject: [PATCH 4/4] transport: server: Throw instead of abort on bounds check failures Instead of crashing the server we will respond with a "Server Error" to the requestor. Fixes #809. --- transport/server.cc | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/transport/server.cc b/transport/server.cc index d54accb63c..c652483210 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -1364,17 +1364,25 @@ void cql_server::response::write_short(uint16_t n) _body.insert(_body.end(), s, s+sizeof(u)); } +template +inline +T cast_if_fits(size_t v) { + size_t max = std::numeric_limits::max(); + if (v > max) { + throw std::runtime_error(sprint("Value to large, %d > %d", v, max)); + } + return static_cast(v); +} + void cql_server::response::write_string(const sstring& s) { - assert(s.size() <= std::numeric_limits::max()); - write_short(s.size()); + write_short(cast_if_fits(s.size())); _body.insert(_body.end(), s.begin(), s.end()); } void cql_server::response::write_long_string(const sstring& s) { - assert(s.size() <= std::numeric_limits::max()); - write_int(s.size()); + write_int(cast_if_fits(s.size())); _body.insert(_body.end(), s.begin(), s.end()); } @@ -1386,8 +1394,7 @@ void cql_server::response::write_uuid(utils::UUID uuid) void cql_server::response::write_string_list(std::vector string_list) { - assert(string_list.size() <= std::numeric_limits::max()); - write_short(string_list.size()); + write_short(cast_if_fits(string_list.size())); for (auto&& s : string_list) { write_string(s); } @@ -1395,15 +1402,13 @@ void cql_server::response::write_string_list(std::vector string_list) void cql_server::response::write_bytes(bytes b) { - assert(b.size() <= std::numeric_limits::max()); - write_int(b.size()); + write_int(cast_if_fits(b.size())); _body.insert(_body.end(), b.begin(), b.end()); } void cql_server::response::write_short_bytes(bytes b) { - assert(b.size() <= std::numeric_limits::max()); - write_short(b.size()); + write_short(cast_if_fits(b.size())); _body.insert(_body.end(), b.begin(), b.end()); } @@ -1436,8 +1441,7 @@ void cql_server::response::write_consistency(db::consistency_level c) void cql_server::response::write_string_map(std::map string_map) { - assert(string_map.size() <= std::numeric_limits::max()); - write_short(string_map.size()); + write_short(cast_if_fits(string_map.size())); for (auto&& s : string_map) { write_string(s.first); write_string(s.second); @@ -1450,8 +1454,7 @@ void cql_server::response::write_string_multimap(std::multimap for (auto it = string_map.begin(), end = string_map.end(); it != end; it = string_map.upper_bound(it->first)) { keys.push_back(it->first); } - assert(keys.size() <= std::numeric_limits::max()); - write_short(keys.size()); + write_short(cast_if_fits(keys.size())); for (auto&& key : keys) { std::vector values; auto range = string_map.equal_range(key);