From 468db35af8875930d424395808ddc9523c57c6f2 Mon Sep 17 00:00:00 2001 From: Michael Penick Date: Wed, 8 Jun 2016 20:36:49 -0700 Subject: [PATCH] Future null dereference cleanup and 2.4.1 prep --- CHANGELOG.md | 9 ++ include/cassandra.h | 2 +- src/future.cpp | 64 +++++------ src/future.hpp | 2 - test/integration_tests/src/test_future.cpp | 128 +++++++++++++++++++++ 5 files changed, 170 insertions(+), 35 deletions(-) create mode 100644 test/integration_tests/src/test_future.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bc1b9473..e5183f984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +2.4.1 +=========== +June 9, 2016 + +Other +-------- +* Fixed issue where `cass_future_get_result()` and similiar methods would + dereference a NULL pointer if a future is set to an error. + 2.4.0 =========== June 1, 2016 diff --git a/include/cassandra.h b/include/cassandra.h index cb65c0d91..26bb5fc4a 100644 --- a/include/cassandra.h +++ b/include/cassandra.h @@ -52,7 +52,7 @@ #define CASS_VERSION_MAJOR 2 #define CASS_VERSION_MINOR 4 -#define CASS_VERSION_PATCH 0 +#define CASS_VERSION_PATCH 1 #define CASS_VERSION_SUFFIX "" #ifdef __cplusplus diff --git a/src/future.cpp b/src/future.cpp index c93b2623b..aa044303b 100644 --- a/src/future.cpp +++ b/src/future.cpp @@ -56,15 +56,11 @@ const CassResult* cass_future_get_result(CassFuture* future) { cass::ResponseFuture* response_future = static_cast(future->from()); - if (response_future->is_error()) return NULL; - cass::SharedRefPtr result(response_future->response()); + if (!result) return NULL; - if (result) { - result->decode_first_row(); - result->inc_ref(); - } - + result->decode_first_row(); + result->inc_ref(); return CassResult::to(result.get()); } @@ -75,16 +71,16 @@ const CassPrepared* cass_future_get_prepared(CassFuture* future) { cass::ResponseFuture* response_future = static_cast(future->from()); - if (response_future->is_error()) return NULL; - cass::SharedRefPtr result(response_future->response()); - if (result && result->kind() == CASS_RESULT_KIND_PREPARED) { - cass::Prepared* prepared = - new cass::Prepared(result, response_future->statement, response_future->schema_metadata); - if (prepared) prepared->inc_ref(); - return CassPrepared::to(prepared); + if (!result || result->kind() != CASS_RESULT_KIND_PREPARED) { + return NULL; } - return NULL; + + cass::Prepared* prepared = new cass::Prepared(result, + response_future->statement, + response_future->schema_metadata); + if (prepared) prepared->inc_ref(); + return CassPrepared::to(prepared); } const CassErrorResult* cass_future_get_error_result(CassFuture* future) { @@ -94,11 +90,14 @@ const CassErrorResult* cass_future_get_error_result(CassFuture* future) { cass::ResponseFuture* response_future = static_cast(future->from()); - if (!response_future->is_error()) return NULL; + cass::SharedRefPtr response(response_future->response()); + if (!response || response->opcode() != CQL_OPCODE_ERROR) { + return NULL; + } - cass::SharedRefPtr error_result(response_future->response()); - if (error_result) error_result->inc_ref(); - return CassErrorResult::to(error_result.get()); + response->inc_ref(); + return CassErrorResult::to( + static_cast(response.get())); } CassError cass_future_error_code(CassFuture* future) { @@ -130,6 +129,7 @@ size_t cass_future_custom_payload_item_count(CassFuture* future) { } cass::SharedRefPtr response( static_cast(future->from())->response()); + if (!response) return 0; return response->custom_payload().size(); } @@ -144,20 +144,20 @@ CassError cass_future_custom_payload_item(CassFuture* future, } cass::SharedRefPtr response( static_cast(future->from())->response()); - if (response) { - const cass::Response::CustomPayloadVec& custom_payload = - response->custom_payload(); - if (index >= custom_payload.size()) { - return CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS; - } - const cass::Response::CustomPayloadItem& item = custom_payload[index]; - *name = item.name.data(); - *name_length = item.name.size(); - *value = reinterpret_cast(item.value.data()); - *value_size = item.value.size(); - return CASS_OK; + if (!response) return CASS_ERROR_LIB_NO_CUSTOM_PAYLOAD; + + const cass::Response::CustomPayloadVec& custom_payload = + response->custom_payload(); + if (index >= custom_payload.size()) { + return CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS; } - return CASS_ERROR_LIB_NO_CUSTOM_PAYLOAD; + + const cass::Response::CustomPayloadItem& item = custom_payload[index]; + *name = item.name.data(); + *name_length = item.name.size(); + *value = reinterpret_cast(item.value.data()); + *value_size = item.value.size(); + return CASS_OK; } } // extern "C" diff --git a/src/future.hpp b/src/future.hpp index 1e27e256e..c190e4099 100644 --- a/src/future.hpp +++ b/src/future.hpp @@ -82,8 +82,6 @@ class Future : public RefCounted { return internal_wait_for(lock, timeout_us); } - bool is_error() { return get_error() != NULL; } - Error* get_error() { ScopedMutex lock(&mutex_); internal_wait(lock); diff --git a/test/integration_tests/src/test_future.cpp b/test/integration_tests/src/test_future.cpp new file mode 100644 index 000000000..31c0179ef --- /dev/null +++ b/test/integration_tests/src/test_future.cpp @@ -0,0 +1,128 @@ +/* + Copyright (c) 2014-2016 DataStax + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "cassandra.h" +#include "test_utils.hpp" + +#include + +struct FuturesTests : public test_utils::MultipleNodesTest { + FuturesTests() : test_utils::MultipleNodesTest(1, 0) { + } +}; + +BOOST_FIXTURE_TEST_SUITE(future, FuturesTests) + +BOOST_AUTO_TEST_CASE(error) +{ + test_utils::CassSessionPtr session(cass_session_new()); + test_utils::CassFuturePtr connect_future(cass_session_connect(session.get(), cluster)); + test_utils::wait_and_check_error(connect_future.get()); + + test_utils::CassStatementPtr statement(cass_statement_new("MALFORMED QUERY", 0)); + test_utils::CassFuturePtr future(cass_session_execute(session.get(), statement.get())); + + BOOST_REQUIRE_NE(cass_future_error_code(future.get()), CASS_OK); + + // Should not be set + BOOST_CHECK(cass_future_get_result(future.get()) == NULL); + BOOST_CHECK(cass_future_get_error_result(future.get()) == NULL); + BOOST_CHECK(cass_future_get_prepared(future.get()) == NULL); + + BOOST_CHECK_EQUAL(cass_future_custom_payload_item_count(future.get()), 0); + { + const char* name; + const cass_byte_t* value; + size_t name_length, value_size; + BOOST_REQUIRE_EQUAL(cass_future_custom_payload_item(future.get(), 0, + &name, &name_length, + &value, &value_size), + CASS_ERROR_LIB_NO_CUSTOM_PAYLOAD); + } +} + +BOOST_AUTO_TEST_CASE(result_response) +{ + test_utils::CassSessionPtr session(cass_session_new()); + test_utils::CassFuturePtr connect_future(cass_session_connect(session.get(), cluster)); + test_utils::wait_and_check_error(connect_future.get()); + + test_utils::CassStatementPtr statement(cass_statement_new("SELECT * FROM system.local", 0)); + test_utils::CassFuturePtr future(cass_session_execute(session.get(), statement.get())); + + // Expected + BOOST_REQUIRE_EQUAL(cass_future_error_code(future.get()), CASS_OK); + BOOST_CHECK(cass_future_get_result(future.get()) != NULL); + + // Should not be set + BOOST_CHECK(cass_future_get_error_result(future.get()) == NULL); + BOOST_CHECK(cass_future_get_prepared(future.get()) == NULL); + + BOOST_CHECK_EQUAL(cass_future_custom_payload_item_count(future.get()), 0); + { + const char* name; + const cass_byte_t* value; + size_t name_length, value_size; + BOOST_REQUIRE_EQUAL(cass_future_custom_payload_item(future.get(), 0, + &name, &name_length, + &value, &value_size), + CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS); + } +} + +BOOST_AUTO_TEST_CASE(prepare_response) +{ + test_utils::CassSessionPtr session(cass_session_new()); + test_utils::CassFuturePtr connect_future(cass_session_connect(session.get(), cluster)); + test_utils::wait_and_check_error(connect_future.get()); + + test_utils::CassFuturePtr future(cass_session_prepare(session.get(), "SELECT * FROM system.local")); + + // Expected + BOOST_REQUIRE_EQUAL(cass_future_error_code(future.get()), CASS_OK); + BOOST_CHECK(cass_future_get_prepared(future.get()) != NULL); + + // This returns a value but probably shouldn't. We should consider fixing + // this, but it could break existing applications. + BOOST_CHECK(cass_future_get_result(future.get()) != NULL); + + // Should not be set + BOOST_CHECK(cass_future_get_error_result(future.get()) == NULL); + + BOOST_CHECK_EQUAL(cass_future_custom_payload_item_count(future.get()), 0); + { + const char* name; + const cass_byte_t* value; + size_t name_length, value_size; + BOOST_REQUIRE_EQUAL(cass_future_custom_payload_item(future.get(), 0, + &name, &name_length, + &value, &value_size), + CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS); + } +} + +BOOST_AUTO_TEST_SUITE_END() +