From: Marcin Siodelski Date: Tue, 18 Jul 2023 05:57:21 +0000 (+0200) Subject: [#2978] Check that mysql pointer is non-null X-Git-Tag: Kea-2.2.1~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e6947b2c4e476ad9d377833d828bbb9536ed3d35;p=thirdparty%2Fkea.git [#2978] Check that mysql pointer is non-null This is a workaround for the libmysqlclient that dereferences mysql ptr in the MYSQL_STMT after reconnect. Kea checks that this pointer is not NULL before using the statement. --- diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc index f4a05637f3..756a620deb 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc @@ -82,18 +82,6 @@ MySqlConfigBackendImpl(const DatabaseConnection::ParameterMap& parameters, } } -MySqlConfigBackendImpl::~MySqlConfigBackendImpl() { - // Free up the prepared statements, ignoring errors. (What would we do - // about them? We're destroying this object and are not really concerned - // with errors on a database connection that is about to go away.) - for (int i = 0; i < conn_.statements_.size(); ++i) { - if (conn_.statements_[i] != NULL) { - (void) mysql_stmt_close(conn_.statements_[i]); - conn_.statements_[i] = NULL; - } - } -} - MySqlBindingPtr MySqlConfigBackendImpl::createBinding(const Triplet& triplet) { if (triplet.unspecified()) { diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h index 2422a601c7..edf31c6c04 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h @@ -113,7 +113,7 @@ public: const db::DbCallback db_reconnect_callback); /// @brief Destructor. - virtual ~MySqlConfigBackendImpl(); + virtual ~MySqlConfigBackendImpl() {}; /// @brief Creates MySQL binding from an @c Optional of integer type. /// diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index af1ffc9400..b39ae54029 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -2937,11 +2937,11 @@ MySqlHostDataSourceImpl::addStatement(MySqlHostContextPtr& ctx, StatementIndex stindex, std::vector& bind) { // Bind the parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], &bind[0]); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), &bind[0]); checkError(ctx, status, stindex, "unable to bind parameters"); // Execute the statement - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); if (status != 0) { // Failure: check for the special case of duplicate entry. @@ -2956,7 +2956,7 @@ MySqlHostDataSourceImpl::addStatement(MySqlHostContextPtr& ctx, // index in the database. Unique indexes are not created in the database // when it may be sometimes allowed to insert duplicated records per // server's configuration. - my_ulonglong numrows = mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]); + my_ulonglong numrows = mysql_stmt_affected_rows(ctx->conn_.getStatement(stindex)); if (numrows == 0) { isc_throw(DuplicateEntry, "Database duplicate entry error"); } @@ -2967,18 +2967,18 @@ MySqlHostDataSourceImpl::delStatement(MySqlHostContextPtr& ctx, StatementIndex stindex, MYSQL_BIND* bind) { // Bind the parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], &bind[0]); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), &bind[0]); checkError(ctx, status, stindex, "unable to bind parameters"); // Execute the statement - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); if (status != 0) { checkError(ctx, status, stindex, "unable to execute"); } // Let's check how many hosts were deleted. - my_ulonglong numrows = mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]); + my_ulonglong numrows = mysql_stmt_affected_rows(ctx->conn_.getStatement(stindex)); return (numrows != 0); } @@ -3046,30 +3046,30 @@ MySqlHostDataSourceImpl::getHostCollection(MySqlHostContextPtr& ctx, bool single) const { // Bind the selection parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], bind); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), bind); checkError(ctx, status, stindex, "unable to bind WHERE clause parameter"); // Set up the MYSQL_BIND array for the data being returned and bind it to // the statement. std::vector outbind = exchange->createBindForReceive(); - status = mysql_stmt_bind_result(ctx->conn_.statements_[stindex], &outbind[0]); + status = mysql_stmt_bind_result(ctx->conn_.getStatement(stindex), &outbind[0]); checkError(ctx, status, stindex, "unable to bind SELECT clause parameters"); // Execute the statement - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to execute"); // Ensure that all the lease information is retrieved in one go to avoid // overhead of going back and forth between client and server. - status = mysql_stmt_store_result(ctx->conn_.statements_[stindex]); + status = mysql_stmt_store_result(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to set up for storing all results"); // Set up the fetch "release" object to release resources associated // with the call to mysql_stmt_fetch when this method exits, then // retrieve the data. mysql_stmt_fetch return value equal to 0 represents // successful data fetch. - MySqlFreeResult fetch_release(ctx->conn_.statements_[stindex]); - while ((status = mysql_stmt_fetch(ctx->conn_.statements_[stindex])) == + MySqlFreeResult fetch_release(ctx->conn_.getStatement(stindex)); + while ((status = mysql_stmt_fetch(ctx->conn_.getStatement(stindex))) == MLM_MYSQL_FETCH_SUCCESS) { try { exchange->processFetchedData(result); diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index dfdb097f5b..a49fff4555 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -1713,7 +1713,7 @@ private: " - invalid statement index" << statement_index_); } - statement_ = conn_.statements_[statement_index_]; + statement_ = conn_.getStatement(statement_index_); } /// @brief Database connection to use to execute the query @@ -1949,11 +1949,11 @@ MySqlLeaseMgr::addLeaseCommon(MySqlLeaseContextPtr& ctx, std::vector& bind) { // Bind the parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], &bind[0]); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), &bind[0]); checkError(ctx, status, stindex, "unable to bind parameters"); // Execute the statement - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); if (status != 0) { // Failure: check for the special case of duplicate entry. If this is @@ -2051,31 +2051,31 @@ MySqlLeaseMgr::getLeaseCollection(MySqlLeaseContextPtr& ctx, if (bind) { // Bind the selection parameters to the statement - status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], bind); + status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), bind); checkError(ctx, status, stindex, "unable to bind WHERE clause parameter"); } // Set up the MYSQL_BIND array for the data being returned and bind it to // the statement. std::vector outbind = exchange->createBindForReceive(); - status = mysql_stmt_bind_result(ctx->conn_.statements_[stindex], &outbind[0]); + status = mysql_stmt_bind_result(ctx->conn_.getStatement(stindex), &outbind[0]); checkError(ctx, status, stindex, "unable to bind SELECT clause parameters"); // Execute the statement - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to execute"); // Ensure that all the lease information is retrieved in one go to avoid // overhead of going back and forth between client and server. - status = mysql_stmt_store_result(ctx->conn_.statements_[stindex]); + status = mysql_stmt_store_result(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to set up for storing all results"); // Set up the fetch "release" object to release resources associated // with the call to mysql_stmt_fetch when this method exits, then // retrieve the data. - MySqlFreeResult fetch_release(ctx->conn_.statements_[stindex]); + MySqlFreeResult fetch_release(ctx->conn_.getStatement(stindex)); int count = 0; - while ((status = mysql_stmt_fetch(ctx->conn_.statements_[stindex])) == 0) { + while ((status = mysql_stmt_fetch(ctx->conn_.getStatement(stindex))) == 0) { try { result.push_back(exchange->getLeaseData()); @@ -2809,16 +2809,16 @@ MySqlLeaseMgr::updateLeaseCommon(MySqlLeaseContextPtr& ctx, const LeasePtr& lease) { // Bind the parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], bind); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), bind); checkError(ctx, status, stindex, "unable to bind parameters"); // Execute - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to execute"); // See how many rows were affected. The statement should only update a // single row. - int affected_rows = mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]); + int affected_rows = mysql_stmt_affected_rows(ctx->conn_.getStatement(stindex)); // Check success case first as it is the most likely outcome. if (affected_rows == 1) { @@ -2951,16 +2951,16 @@ MySqlLeaseMgr::deleteLeaseCommon(StatementIndex stindex, MySqlLeaseContextPtr ctx = get_context.ctx_; // Bind the input parameters to the statement - int status = mysql_stmt_bind_param(ctx->conn_.statements_[stindex], bind); + int status = mysql_stmt_bind_param(ctx->conn_.getStatement(stindex), bind); checkError(ctx, status, stindex, "unable to bind WHERE clause parameter"); // Execute - status = MysqlExecuteStatement(ctx->conn_.statements_[stindex]); + status = MysqlExecuteStatement(ctx->conn_.getStatement(stindex)); checkError(ctx, status, stindex, "unable to execute"); // See how many rows were affected. Note that the statement may delete // multiple rows. - return (static_cast(mysql_stmt_affected_rows(ctx->conn_.statements_[stindex]))); + return (static_cast(mysql_stmt_affected_rows(ctx->conn_.getStatement(stindex)))); } bool diff --git a/src/lib/mysql/mysql_connection.h b/src/lib/mysql/mysql_connection.h index f68a42c5de..e1342464bf 100644 --- a/src/lib/mysql/mysql_connection.h +++ b/src/lib/mysql/mysql_connection.h @@ -304,6 +304,24 @@ public: /// @brief Clears prepared statements and text statements. void clearStatements(); + /// @brief Returns a prepared statement by an index + /// + /// @tparam StatementIndex Type of the statement index enum. + /// + /// @param index Statement index. + /// @return Pointer to the prepared statement. + /// @throw isc::db::DbConnectionUnusable when the @c mysql pointer in the + /// returned statement is NULL; it may be the result of the database + /// connectivity loss. + template + MYSQL_STMT* getStatement(StatementIndex index) const { + if (statements_[index]->mysql == 0) { + isc_throw(db::DbConnectionUnusable, + "MySQL pointer for the prepared statement is NULL as a result of connectivity loss"); + } + return (statements_[index]); + } + /// @brief Open Database /// /// Opens the database using the information supplied in the parameters @@ -436,7 +454,7 @@ public: int status = 0; if (!in_bind_vec.empty()) { // Bind parameters to the prepared statement. - status = mysql_stmt_bind_param(statements_[index], + status = mysql_stmt_bind_param(getStatement(index), in_bind_vec.empty() ? 0 : &in_bind_vec[0]); checkError(status, index, "unable to bind parameters for select"); } @@ -447,20 +465,20 @@ public: out_bind_vec.push_back(out_binding->getMySqlBinding()); } if (!out_bind_vec.empty()) { - status = mysql_stmt_bind_result(statements_[index], &out_bind_vec[0]); + status = mysql_stmt_bind_result(getStatement(index), &out_bind_vec[0]); checkError(status, index, "unable to bind result parameters for select"); } // Execute query. - status = MysqlExecuteStatement(statements_[index]); + status = MysqlExecuteStatement(getStatement(index)); checkError(status, index, "unable to execute"); - status = mysql_stmt_store_result(statements_[index]); + status = mysql_stmt_store_result(getStatement(index)); checkError(status, index, "unable to set up for storing all results"); // Fetch results. - MySqlFreeResult fetch_release(statements_[index]); - while ((status = mysql_stmt_fetch(statements_[index])) == + MySqlFreeResult fetch_release(getStatement(index)); + while ((status = mysql_stmt_fetch(getStatement(index))) == MLM_MYSQL_FETCH_SUCCESS) { try { // For each returned row call user function which should @@ -511,12 +529,12 @@ public: } // Bind the parameters to the statement - int status = mysql_stmt_bind_param(statements_[index], + int status = mysql_stmt_bind_param(getStatement(index), in_bind_vec.empty() ? 0 : &in_bind_vec[0]); checkError(status, index, "unable to bind parameters"); // Execute the statement - status = MysqlExecuteStatement(statements_[index]); + status = MysqlExecuteStatement(getStatement(index)); if (status != 0) { // Failure: check for the special case of duplicate entry. @@ -555,12 +573,12 @@ public: } // Bind the parameters to the statement - int status = mysql_stmt_bind_param(statements_[index], + int status = mysql_stmt_bind_param(getStatement(index), in_bind_vec.empty() ? 0 : &in_bind_vec[0]); checkError(status, index, "unable to bind parameters"); // Execute the statement - status = MysqlExecuteStatement(statements_[index]); + status = MysqlExecuteStatement(getStatement(index)); if (status != 0) { // Failure: check for the special case of duplicate entry. @@ -581,7 +599,7 @@ public: } // Let's return how many rows were affected. - return (static_cast(mysql_stmt_affected_rows(statements_[index]))); + return (static_cast(mysql_stmt_affected_rows(getStatement(index)))); } /// @brief Commits current transaction @@ -731,14 +749,14 @@ private: template void setIntParameterValue(const std::string& name, int64_t min, int64_t max, T& value); -public: - /// @brief Prepared statements /// - /// This field is public, because it is used heavily from MySqlConnection - /// and will be from MySqlHostDataSource. + /// The statements should be accessed using the @c getStatement method because + /// it checks whether the returned statement is valid. std::vector statements_; +public: + /// @brief Raw text of statements /// /// This field is public, because it is used heavily from MySqlConnection diff --git a/src/lib/mysql/tests/mysql_connection_unittest.cc b/src/lib/mysql/tests/mysql_connection_unittest.cc index cc2b21d3ea..b2ac654552 100644 --- a/src/lib/mysql/tests/mysql_connection_unittest.cc +++ b/src/lib/mysql/tests/mysql_connection_unittest.cc @@ -6,6 +6,7 @@ #include +#include #include #include #include @@ -741,6 +742,16 @@ TEST_F(MySqlConnectionTest, writeTimeoutZero) { #endif // HAVE_MYSQL_GET_OPTION + +// Tests that the statement can be accessed by index. +TEST_F(MySqlConnectionTest, getStatement) { + auto statement0 = conn_.getStatement(0); + ASSERT_TRUE(statement0); + auto statement1 = conn_.getStatement(1); + ASSERT_TRUE(statement1); + EXPECT_NE(statement0, statement1); +} + TEST_F(MySqlConnectionWithPrimaryKeyTest, select) { select(); }