Future null dereference cleanup and 2.4.1 prep #2

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

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

src/future.cpp

@@ -56,15 +56,11 @@ const CassResult* cass_future_get_result(CassFuture* future) {
   cass::ResponseFuture* response_future =
       static_cast<cass::ResponseFuture*>(future->from());
 
-  if (response_future->is_error()) return NULL;
-
   cass::SharedRefPtr<cass::ResultResponse> 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<cass::ResponseFuture*>(future->from());
 
-  if (response_future->is_error()) return NULL;
-
   cass::SharedRefPtr<cass::ResultResponse> 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<cass::ResponseFuture*>(future->from());
 
-  if (!response_future->is_error()) return NULL;
+  cass::SharedRefPtr<cass::Response> response(response_future->response());
+  if (!response || response->opcode() != CQL_OPCODE_ERROR) {
+    return NULL;
+  }
 
-  cass::SharedRefPtr<cass::ErrorResponse> 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<cass::ErrorResponse*>(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<cass::Response> response(
         static_cast<cass::ResponseFuture*>(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<cass::Response> response(
         static_cast<cass::ResponseFuture*>(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<const cass_byte_t*>(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<const cass_byte_t*>(item.value.data());
+  *value_size = item.value.size();
+  return CASS_OK;
 }
 
 } // extern "C"

src/future.hpp

@@ -82,8 +82,6 @@ class Future : public RefCounted<Future> {
     return internal_wait_for(lock, timeout_us);
   }
 
-  bool is_error() { return get_error() != NULL; }
-
   Error* get_error() {
     ScopedMutex lock(&mutex_);
     internal_wait(lock);

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 <boost/test/unit_test.hpp>
+#include <boost/test/debug.hpp>
+#include <boost/lexical_cast.hpp>
+#include <boost/cstdint.hpp>
+#include <boost/format.hpp>
+#include <boost/thread.hpp>
+#include <boost/scoped_ptr.hpp>
+#include <boost/thread/mutex.hpp>
+#include <boost/thread/condition.hpp>
+
+#include "cassandra.h"
+#include "test_utils.hpp"
+
+#include <sstream>
+
+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()
+