From 0ed3347433c814f381921b90263b5ad1ea81e127 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Mon, 23 Sep 2024 15:12:26 +0500 Subject: [PATCH 1/2] Removed Client_Session.h again --- include/Client_Session.h | 123 --------------------------------------- 1 file changed, 123 deletions(-) delete mode 100644 include/Client_Session.h diff --git a/include/Client_Session.h b/include/Client_Session.h deleted file mode 100644 index 1e6cdd401..000000000 --- a/include/Client_Session.h +++ /dev/null @@ -1,123 +0,0 @@ -#ifndef __CLASS_CLIENT_SESSION_H -#define __CLASS_CLIENT_SESSION_H - -#include -#include - -#include "proxysql.h" -#include "cpp.h" -#include "MySQL_Variables.h" - -//#include "../deps/json/json.hpp" -//using json = nlohmann::json; - -#ifndef PROXYJSON -#define PROXYJSON -#include "../deps/json/json_fwd.hpp" -#endif // PROXYJSON - -class MySQL_Session; -class PgSQL_Session; - -#if 0 -// this code was moved into Base_Session.h -/** - * @class Session_Regex - * @brief Encapsulates regex operations for session handling. - * - * This class is used for matching patterns in SQL queries, specifically for - * settings like sql_log_bin, sql_mode, and time_zone. - * See issues #509 , #815 and #816 - */ -class Session_Regex { -private: - void* opt; - void* re; - char* s; -public: - Session_Regex(char* p); - ~Session_Regex(); - bool match(char* m); -}; -#endif // 0 - -template -class TypeSelector { -}; - -template -class TypeSelector { -public: - TypeSelector(T* _ptr) : ptr(_ptr) { } - ~TypeSelector() {} - TypeSelector& operator=(T* _ptr) { - ptr = _ptr; - return *this; - } - - T* operator->() { - return ptr; - } - bool operator==(T* _ptr) { return (ptr == _ptr); } - T& operator*() const noexcept { return *ptr; } - explicit operator bool() { return ptr != nullptr; } - operator T* () { - return ptr; - } - -private: - T* ptr; -}; - -/* Issues with forward class declaration -template -using Client_Session = TypeSelector; -*/ -template -class Client_Session : public TypeSelector { -public: - //Client_Session(const Client_Session&) = default; - //Client_Session(Client_Session&&) = default; - //Client_Session& operator=(const Client_Session&) = default; - //Client_Session& operator=(Client_Session&&) = default; - //~Client_Session() = default; - using TypeSelector::TypeSelector; -}; -#define TO_CLIENT_SESSION(sess) Client_Session(sess) - -/* Issues with forward class declaration -template -using Query_Info_T = TypeSelector; -*/ -template -class Query_Info_T : public TypeSelector { -public: - using TypeSelector::TypeSelector; -}; -#define TO_QUERY_INFO(query_info) Query_Info_T(query_info) - -/* Issues with forward class declaration -template -using Data_Stream_T = TypeSelector; -*/ -template -class Data_Stream_T : public TypeSelector { -public: - using TypeSelector::TypeSelector; -}; -#define TO_DATA_STREAM(data_stream) Data_Stream_T(data_stream) - -/* Issues with forward class declaration -template -using Connection_Info_T = TypeSelector; -*/ -template -class Connection_Info_T : public TypeSelector { -public: - using TypeSelector::TypeSelector; -}; -#define TO_CONNECTION_INFO(connection_info) Connection_Info_T(connection_info) - -std::string proxysql_session_type_str(enum proxysql_session_type session_type); - -#endif /* __CLASS_CLIENT_SESSION_H */ From 5f218441a3187d5c0439c2bdcad5250c69e47eda Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Tue, 24 Sep 2024 23:23:42 +0500 Subject: [PATCH 2/2] Return an error if feature is not supported, instead of crashing (assert) --- lib/PgSQL_Connection.cpp | 1 + lib/PgSQL_Session.cpp | 54 +++++-- .../pgsql-unsupported_feature_test-t.cpp | 135 ++++++++++++++++++ 3 files changed, 182 insertions(+), 8 deletions(-) create mode 100644 test/tap/tests/pgsql-unsupported_feature_test-t.cpp diff --git a/lib/PgSQL_Connection.cpp b/lib/PgSQL_Connection.cpp index a51a11b60..2798a0a6d 100644 --- a/lib/PgSQL_Connection.cpp +++ b/lib/PgSQL_Connection.cpp @@ -1807,6 +1807,7 @@ PG_ASYNC_ST PgSQL_Connection::handler(short event) { case PGRES_COPY_IN: case PGRES_COPY_BOTH: // NOT IMPLEMENTED + proxy_error("COPY not supported\n"); assert(0); break; case PGRES_BAD_RESPONSE: diff --git a/lib/PgSQL_Session.cpp b/lib/PgSQL_Session.cpp index edac2a69d..d6e8e0d9c 100644 --- a/lib/PgSQL_Session.cpp +++ b/lib/PgSQL_Session.cpp @@ -959,6 +959,24 @@ bool PgSQL_Session::handler_special_queries(PtrSize_t* pkt) { return true; } } + // Unsupported Features: + // COPY + if (pkt->size > (5 + 5) && strncasecmp((char*)"COPY ", (char*)pkt->ptr + 5, 5) == 0) { + client_myds->DSS = STATE_QUERY_SENT_NET; + client_myds->myprot.generate_error_packet(true, true, "Feature not supported", PGSQL_ERROR_CODES::ERRCODE_FEATURE_NOT_SUPPORTED, + false, true); + //client_myds->DSS = STATE_SLEEP; + //status = WAITING_CLIENT_DATA; + if (mirror == false) { + RequestEnd(NULL); + } else { + client_myds->DSS = STATE_SLEEP; + status = WAITING_CLIENT_DATA; + } + l_free(pkt->size, pkt->ptr); + return true; + } + // if (pkt->size > (5 + 18) && strncasecmp((char*)"PROXYSQL INTERNAL ", (char*)pkt->ptr + 5, 18) == 0) { return_proxysql_internal(pkt); return true; @@ -3307,17 +3325,24 @@ int PgSQL_Session::get_pkts_from_client(bool& wrong_pass, PtrSize_t& pkt) { c = *((unsigned char*)pkt.ptr + 0); if (c == 'Q') { handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY___not_mysql(pkt); - } - else if (c == 'X') { + } else if (c == 'X') { //proxy_debug(PROXY_DEBUG_MYSQL_COM, 5, "Got COM_QUIT packet\n"); //if (GloPgSQL_Logger) { GloPgSQL_Logger->log_audit_entry(PROXYSQL_MYSQL_AUTH_QUIT, this, NULL); } l_free(pkt.size, pkt.ptr); handler_ret = -1; return handler_ret; - } - else { - proxy_error("Not implemented yet"); - assert(0); + } else if (c == 'P' || c == 'B' || c == 'D' || c == 'E') { + l_free(pkt.size, pkt.ptr); + continue; + } else { + proxy_error("Not implemented yet. Message type:'%c'\n", c); + client_myds->setDSS_STATE_QUERY_SENT_NET(); + client_myds->myprot.generate_error_packet(true, true, "Feature not supported", PGSQL_ERROR_CODES::ERRCODE_FEATURE_NOT_SUPPORTED, + false, true); + l_free(pkt.size, pkt.ptr); + client_myds->DSS = STATE_SLEEP; + //handler_ret = -1; + return handler_ret; } } else { @@ -3515,9 +3540,22 @@ int PgSQL_Session::get_pkts_from_client(bool& wrong_pass, PtrSize_t& pkt) { handler_ret = -1; return handler_ret; break; + case 'P': + case 'B': + case 'D': + case 'E': + //ignore + l_free(pkt.size, pkt.ptr); + continue; + case 'S': default: - proxy_error("Not implemented yet"); - assert(0); + proxy_error("Not implemented yet. Message type:'%c'\n", c); + client_myds->setDSS_STATE_QUERY_SENT_NET(); + client_myds->myprot.generate_error_packet(true, true, "Feature not supported", PGSQL_ERROR_CODES::ERRCODE_FEATURE_NOT_SUPPORTED, + false, true); + l_free(pkt.size, pkt.ptr); + client_myds->DSS = STATE_SLEEP; + return handler_ret; } } break; diff --git a/test/tap/tests/pgsql-unsupported_feature_test-t.cpp b/test/tap/tests/pgsql-unsupported_feature_test-t.cpp new file mode 100644 index 000000000..e52d9fafb --- /dev/null +++ b/test/tap/tests/pgsql-unsupported_feature_test-t.cpp @@ -0,0 +1,135 @@ +/** + * @file pgsql-unsupported_feature_test-t.cpp + * @brief Ensures that ProxySQL does not crash and maintains the connection/session integrity when unsupported queries are executed. + * Currently validates: + * 1) Prepare Statement + * 2) COPY + */ + +#include +#include + +#include "libpq-fe.h" +#include "command_line.h" +#include "tap.h" +#include "utils.h" + +CommandLine cl; + +PGconn* create_new_connection(bool with_ssl) { + std::stringstream ss; + + ss << "host=" << cl.pgsql_host << " port=" << cl.pgsql_port; + ss << " user=" << cl.pgsql_username << " password=" << cl.pgsql_password; + + if (with_ssl) { + ss << " sslmode=require"; + } else { + ss << " sslmode=disable"; + } + + PGconn* conn = PQconnectdb(ss.str().c_str()); + const bool res = (conn && PQstatus(conn) == CONNECTION_OK); + ok(res, "Connection created successfully. %s", PQerrorMessage(conn)); + + if (res) return conn; + + PQfinish(conn); + return nullptr; +} + +void check_transaction_state(PGconn* conn) { + PGresult* res; + + // Check if the transaction is still active + res = PQexec(conn, "SELECT 1"); + ok(PQresultStatus(res) == PGRES_TUPLES_OK && PQtransactionStatus(conn) == PQTRANS_INTRANS, + "Transaction state was not affected by the error. %s", PQerrorMessage(conn)); + PQclear(res); +} + +void check_prepared_statement_binary(PGconn* conn) { + PGresult* res; + const char* paramValues[1] = { "1" }; + + // Start a transaction + res = PQexec(conn, "BEGIN"); + if (PQresultStatus(res) != PGRES_COMMAND_OK) { + BAIL_OUT("Could not start transaction. %s", PQerrorMessage(conn)); + } + PQclear(res); + + // Test: Prepare a statement (using binary mode) + res = PQprepare(conn, "myplan", "SELECT $1::int", 1, NULL); + ok(PQresultStatus(res) != PGRES_COMMAND_OK, "Prepare statement failed. %s", PQerrorMessage(conn)); + PQclear(res); + + // Execute the prepared statement using binary protocol + res = PQexecPrepared(conn, "myplan", 1, paramValues, NULL, NULL, 1); // Binary result format (1) + ok(PQresultStatus(res) != PGRES_COMMAND_OK && PQresultStatus(res) != PGRES_TUPLES_OK, "Prepare statements are not supported for PostgreSQL: %s", PQerrorMessage(conn)); + PQclear(res); + + // Check if the transaction state is still active + check_transaction_state(conn); + + // End the transaction + res = PQexec(conn, "ROLLBACK"); + PQclear(res); +} + +void check_copy_binary(PGconn* conn) { + PGresult* res; + + // Start a transaction + res = PQexec(conn, "BEGIN"); + if (PQresultStatus(res) != PGRES_COMMAND_OK) { + BAIL_OUT("Could not start transaction. %s", PQerrorMessage(conn)); + } + PQclear(res); + + // Test: COPY binary format + res = PQexec(conn, "COPY (SELECT 1) TO STDOUT (FORMAT BINARY)"); + ok(PQresultStatus(res) != PGRES_COPY_OUT, "COPY binary command failed to start. %s", PQerrorMessage(conn)); + PQclear(res); + + // Attempt to fetch data in binary mode, expect it to fail + char buffer[256]; + int ret = PQgetCopyData(conn, (char**)&buffer, 1); // Binary mode (1) + ok(ret == -2, "COPY in binary mode should have failed. %s", PQerrorMessage(conn)); + + // Check if the transaction state is still active + check_transaction_state(conn); + + // End the transaction + res = PQexec(conn, "ROLLBACK"); + PQclear(res); +} + +void execute_tests(bool with_ssl) { + PGconn* conn = create_new_connection(with_ssl); + + if (conn == nullptr) + return; + + // Test 1: Prepared Statement in binary mode + check_prepared_statement_binary(conn); + + // Test 2: COPY in binary mode + check_copy_binary(conn); + + // Close the connection + + PQfinish(conn); +} + +int main(int argc, char** argv) { + + plan(7); // Total number of tests planned + + if (cl.getEnv()) + return exit_status(); + + execute_tests(false); // without SSL + + return exit_status(); +}