From 944b0efcaf3f81c2514578c4bb6d1a8d8b7cb325 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 02:27:58 +0500 Subject: [PATCH 1/6] Fixed extended query message parser Now correctly handles empty (len=0) parameter length.. --- include/PgSQL_Extended_Query_Message.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/include/PgSQL_Extended_Query_Message.h b/include/PgSQL_Extended_Query_Message.h index 92ddcd5f5..67c098fca 100644 --- a/include/PgSQL_Extended_Query_Message.h +++ b/include/PgSQL_Extended_Query_Message.h @@ -158,12 +158,18 @@ public: } current += sizeof(uint32_t); - out->len = (len == 0xFFFFFFFF) ? -1 : static_cast(len); - out->value = (len == 0xFFFFFFFF) ? nullptr : current; + constexpr uint32_t PGSQL_PARAM_NULL = 0xFFFFFFFF; + + if (len != PGSQL_PARAM_NULL && len > INT32_MAX) { + return false; // malformed + } + + out->len = (len == PGSQL_PARAM_NULL) ? -1 : static_cast(len); + out->value = (out->len > 0) ? current : nullptr; // Advance pointer if not NULL if (out->len > 0) { - current += len; + current += out->len; } } remaining--; From 786fb370d67781bca287f9a3acf90b3aa41022d5 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 02:35:25 +0500 Subject: [PATCH 2/6] Fix parameter length misalignment in PQsendQueryPrepared For text parameters (format=0), respect paramLengths[i] if provided, otherwise fall back to strlen(). --- deps/Makefile | 3 ++- deps/postgresql/bind_fmt_text.patch | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 deps/postgresql/bind_fmt_text.patch diff --git a/deps/Makefile b/deps/Makefile index 87d2a20e8..6dacbcf82 100644 --- a/deps/Makefile +++ b/deps/Makefile @@ -307,7 +307,8 @@ postgresql/postgresql/src/interfaces/libpq/libpq.a: cd postgresql/postgresql && patch -p0 < ../get_result_from_pgconn.patch cd postgresql/postgresql && patch -p0 < ../handle_row_data.patch cd postgresql/postgresql && patch -p0 < ../fmt_err_msg.patch - #cd postgresql/postgresql && LD_LIBRARY_PATH="$(shell pwd)/libssl/openssl" ./configure --with-ssl=openssl --with-includes="$(shell pwd)/libssl/openssl/include/" --with-libraries="$(shell pwd)/libssl/openssl/" --without-readline --enable-debug CFLAGS="-ggdb -O0 -fno-omit-frame-pointer" CPPFLAGS="-g -O0" + cd postgresql/postgresql && patch -p0 < ../bind_fmt_text.patch + #cd postgresql/postgresql && LD_LIBRARY_PATH="$(SSL_LDIR)" ./configure --with-ssl=openssl --with-includes="$(SSL_IDIR)" --with-libraries="$(SSL_LDIR)" --without-readline --enable-debug CFLAGS="-ggdb -O0 -fno-omit-frame-pointer" CPPFLAGS="-g -O0" cd postgresql/postgresql && LD_LIBRARY_PATH="$(SSL_LDIR)" ./configure --with-ssl=openssl --with-includes="$(SSL_IDIR)" --with-libraries="$(SSL_LDIR)" --without-readline cd postgresql/postgresql/src/interfaces/libpq && CC=${CC} CXX=${CXX} ${MAKE} MAKELEVEL=0 #cd postgresql/postgresql && CC=${CC} CXX=${CXX} ${MAKE} -f src/interfaces/libpq/Makefile all diff --git a/deps/postgresql/bind_fmt_text.patch b/deps/postgresql/bind_fmt_text.patch new file mode 100644 index 000000000..de0d9947c --- /dev/null +++ b/deps/postgresql/bind_fmt_text.patch @@ -0,0 +1,22 @@ +diff --git src/interfaces/libpq/fe-exec.c src/interfaces/libpq/fe-exec.c +index d572bde..99bab2c 100644 +--- src/interfaces/libpq/fe-exec.c ++++ src/interfaces/libpq/fe-exec.c +@@ -1855,8 +1855,15 @@ PQsendQueryGuts(PGconn *conn, + } + else + { +- /* text parameter, do not use paramLengths */ +- nbytes = strlen(paramValues[i]); ++ /* text parameter */ ++ if (paramLengths && paramLengths[i] >= 0) ++ { ++ nbytes = paramLengths[i]; // use supplied length ++ } ++ else ++ { ++ nbytes = strlen(paramValues[i]); // fallback to strlen ++ } + } + if (pqPutInt(nbytes, 4, conn) < 0 || + pqPutnchar(paramValues[i], nbytes, conn) < 0) From 8a1bf832c5268589ec0fbd76f0ed1efae01cbe8b Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 13:25:41 +0500 Subject: [PATCH 3/6] Add regression test --- ...sql-reg_test_5140_bind_param_fmt_mix-t.cpp | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp diff --git a/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp b/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp new file mode 100644 index 000000000..1d9d2376f --- /dev/null +++ b/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp @@ -0,0 +1,129 @@ +/** + * @file pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp + * @brief Regression test to check libpq's handling of mixed text and binary parameter. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include "libpq-fe.h" +#include "command_line.h" +#include "tap.h" +#include "utils.h" + +CommandLine cl; + +using PGConnPtr = std::unique_ptr; + +enum ConnType { + ADMIN, + BACKEND +}; + +PGConnPtr createNewConnection(ConnType conn_type, const std::string& options = "", bool with_ssl = false) { + + const char* host = (conn_type == BACKEND) ? cl.pgsql_host : cl.pgsql_admin_host; + int port = (conn_type == BACKEND) ? cl.pgsql_port : cl.pgsql_admin_port; + const char* username = (conn_type == BACKEND) ? cl.pgsql_username : cl.admin_username; + const char* password = (conn_type == BACKEND) ? cl.pgsql_password : cl.admin_password; + + std::stringstream ss; + + ss << "host=" << host << " port=" << port; + ss << " user=" << username << " password=" << password; + ss << (with_ssl ? " sslmode=require" : " sslmode=disable"); + + if (options.empty() == false) { + ss << " options='" << options << "'"; + } + + PGconn* conn = PQconnectdb(ss.str().c_str()); + if (PQstatus(conn) != CONNECTION_OK) { + fprintf(stderr, "Connection failed to '%s': %s", (conn_type == BACKEND ? "Backend" : "Admin"), PQerrorMessage(conn)); + PQfinish(conn); + return PGConnPtr(nullptr, &PQfinish); + } + return PGConnPtr(conn, &PQfinish); +} + +int main(int argc, char** argv) { + + if (cl.getEnv()) + return exit_status(); + + plan(2); + + auto conn = createNewConnection(BACKEND); + if (!conn) { + diag("connection failed"); + return 1; + } + + + // Ensure we have a test table + PGresult* res = PQexec(conn.get(), + "CREATE TEMP TABLE reg_test_5140 (col1 text,col2 int)" + ); + if (PQresultStatus(res) != PGRES_COMMAND_OK) { + diag(PQerrorMessage(conn.get())); + PQclear(res); + return 1; + } + PQclear(res); + + // Prepare a statement with 2 parameters + res = PQprepare(conn.get(), "stmt_test_bind_5140", + "INSERT INTO reg_test_5140 VALUES ($1, $2)", 2, nullptr); + if (PQresultStatus(res) != PGRES_COMMAND_OK) { + diag(PQerrorMessage(conn.get())); + PQclear(res); + return 1; + } + PQclear(res); + + // Parameter values: + // col1 = "ABCDEFGHIJKLMN" (length 14) + // col3 = NULL (-1) + const char* values[2]; + int lengths[2]; + int formats[2]; + + values[0] = "ABCDEFGHIJKLMN"; // 14 chars + lengths[0] = 14; + formats[0] = 0; // Text Format + + values[1] = nullptr; // NULL + lengths[1] = -1; + formats[1] = 1; // Binary Format + + res = PQexecPrepared(conn.get(), "stmt_test_bind_5140", 2, values, lengths, formats, 0); + if (PQresultStatus(res) != PGRES_COMMAND_OK) { + diag(PQerrorMessage(conn.get())); + PQclear(res); + return 1; + } + PQclear(res); + + // Verify row inserted as expected + res = PQexec(conn.get(), "SELECT col1, col2 FROM reg_test_5140"); + if (PQresultStatus(res) != PGRES_TUPLES_OK) { + diag(PQerrorMessage(conn.get())); + PQclear(res); + return 1; + } + + char const* c1 = PQgetvalue(res, 0, 0); + bool isnull_c2 = PQgetisnull(res, 0, 2); + + ok(std::string(c1) == "ABCDEFGHIJKLMN", "col1 length 14 parsed correctly"); + ok(isnull_c2, "col3 NULL parsed correctly"); + + PQclear(res); + + return exit_status(); +} From 8060f98c286e795653425af1a78aa0e421218246 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 15:21:26 +0500 Subject: [PATCH 4/6] Improve Parser --- include/PgSQL_Extended_Query_Message.h | 33 ++++++++++++++++++++++---- lib/PgSQL_Connection.cpp | 1 - 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/include/PgSQL_Extended_Query_Message.h b/include/PgSQL_Extended_Query_Message.h index 67c098fca..c453c2908 100644 --- a/include/PgSQL_Extended_Query_Message.h +++ b/include/PgSQL_Extended_Query_Message.h @@ -105,6 +105,10 @@ struct PgSQL_Param_Value { * If count is non-zero but the buffer is invalid (malformed packet), this is detected and handled * during parsing before constructing the reader. * + * Ownership/lifetime: The reader only keeps raw pointers into the caller-provided buffer and + * does not allocate. Any T that exposes pointers (e.g., PgSQL_Param_Value) references the original + * buffer; ensure it outlives the reader and any consumer that accesses those pointers. + * * @tparam T The type of field to read (e.g., uint32_t, uint16_t, PgSQL_Param_Value). */ template @@ -136,6 +140,16 @@ public: * * For uint32_t and uint16_t, reads the value in big-endian order. * For PgSQL_Param_Value, reads the length and value pointer, handling NULL values. + * + * PgSQL_Param_Value decoding details: + * - Reads a 4-byte big-endian length (uint32_t). + * * 0xFFFFFFFF => SQL NULL: out->len = -1 and out->value = nullptr. + * * 0x00000000 => empty, non-NULL value: out->len = 0 and out->value is set + * to an empty string. + * * Otherwise => non-empty: out->len = static_cast(len) and out->value points + * to the next len bytes starting at the current parse position. + * - The internal cursor advances by out->len bytes only when out->len > 0. + * - The pointer returned is non-owning and valid only as long as the source buffer is alive. */ bool next(T* out) { if (remaining == 0) return false; @@ -151,7 +165,7 @@ public: } current += sizeof(uint16_t); } else if constexpr (std::is_same_v) { - // Read length (big-endian) + // Read length (big-endian) from the wire uint32_t len; if (!get_uint32be(current, &len)) { return false; @@ -161,13 +175,24 @@ public: constexpr uint32_t PGSQL_PARAM_NULL = 0xFFFFFFFF; if (len != PGSQL_PARAM_NULL && len > INT32_MAX) { - return false; // malformed + return false; // Malformed: length does not fit into int32_t } out->len = (len == PGSQL_PARAM_NULL) ? -1 : static_cast(len); - out->value = (out->len > 0) ? current : nullptr; - // Advance pointer if not NULL + // Value pointer semantics: + // - NULL (len == -1): pointer is nullptr. + // - Empty (len == 0): pointer is set to empty string. + // - Non-empty (len > 0): pointer to the next 'len' bytes. + if (out->len == -1) { + out->value = nullptr; // NULL + } else if (out->len == 0) { + out->value = reinterpret_cast(""); // empty string + } else { + out->value = current; + } + + // Advance pointer only for non-empty values if (out->len > 0) { current += out->len; } diff --git a/lib/PgSQL_Connection.cpp b/lib/PgSQL_Connection.cpp index 494d300cc..02c507b4c 100644 --- a/lib/PgSQL_Connection.cpp +++ b/lib/PgSQL_Connection.cpp @@ -1864,7 +1864,6 @@ void PgSQL_Connection::stmt_execute_start() { set_error(PGSQL_ERROR_CODES::ERRCODE_INVALID_PARAMETER_VALUE, "Failed to read param format", false); return; - return; } param_formats[i] = format; } From ee4cd98a401e9b485b46c649ee5468349df095a1 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 15:22:01 +0500 Subject: [PATCH 5/6] Improve regression test --- ...sql-reg_test_5140_bind_param_fmt_mix-t.cpp | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp b/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp index 1d9d2376f..98e61a7e9 100644 --- a/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp +++ b/test/tap/tests/pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp @@ -1,7 +1,7 @@ /** * @file pgsql-reg_test_5140_bind_param_fmt_mix-t.cpp * @brief Regression test to check libpq's handling of mixed text and binary parameter. - * + * */ #include @@ -26,7 +26,7 @@ enum ConnType { }; PGConnPtr createNewConnection(ConnType conn_type, const std::string& options = "", bool with_ssl = false) { - + const char* host = (conn_type == BACKEND) ? cl.pgsql_host : cl.pgsql_admin_host; int port = (conn_type == BACKEND) ? cl.pgsql_port : cl.pgsql_admin_port; const char* username = (conn_type == BACKEND) ? cl.pgsql_username : cl.admin_username; @@ -39,7 +39,7 @@ PGConnPtr createNewConnection(ConnType conn_type, const std::string& options = " ss << (with_ssl ? " sslmode=require" : " sslmode=disable"); if (options.empty() == false) { - ss << " options='" << options << "'"; + ss << " options='" << options << "'"; } PGconn* conn = PQconnectdb(ss.str().c_str()); @@ -56,7 +56,7 @@ int main(int argc, char** argv) { if (cl.getEnv()) return exit_status(); - plan(2); + plan(4); auto conn = createNewConnection(BACKEND); if (!conn) { @@ -64,23 +64,22 @@ int main(int argc, char** argv) { return 1; } - // Ensure we have a test table PGresult* res = PQexec(conn.get(), - "CREATE TEMP TABLE reg_test_5140 (col1 text,col2 int)" + "CREATE TEMP TABLE reg_test_5140 (col1 text,col2 int, col3 text, col4 text)" ); if (PQresultStatus(res) != PGRES_COMMAND_OK) { - diag(PQerrorMessage(conn.get())); + diag("Error: %s", PQerrorMessage(conn.get())); PQclear(res); return 1; } PQclear(res); - // Prepare a statement with 2 parameters + // Prepare a statement with 4 parameters res = PQprepare(conn.get(), "stmt_test_bind_5140", - "INSERT INTO reg_test_5140 VALUES ($1, $2)", 2, nullptr); + "INSERT INTO reg_test_5140 VALUES ($1, $2, $3, $4)", 4, nullptr); if (PQresultStatus(res) != PGRES_COMMAND_OK) { - diag(PQerrorMessage(conn.get())); + diag("Error: %s", PQerrorMessage(conn.get())); PQclear(res); return 1; } @@ -88,10 +87,12 @@ int main(int argc, char** argv) { // Parameter values: // col1 = "ABCDEFGHIJKLMN" (length 14) - // col3 = NULL (-1) - const char* values[2]; - int lengths[2]; - int formats[2]; + // col2 = NULL (-1) + // col3 = empty string (length 0) + // col4 = binary data (length 0) + const char* values[4]; + int lengths[4]; + int formats[4]; values[0] = "ABCDEFGHIJKLMN"; // 14 chars lengths[0] = 14; @@ -101,27 +102,40 @@ int main(int argc, char** argv) { lengths[1] = -1; formats[1] = 1; // Binary Format - res = PQexecPrepared(conn.get(), "stmt_test_bind_5140", 2, values, lengths, formats, 0); + values[2] = ""; // empty string + lengths[2] = 0; + formats[2] = 0; // Text Format + + values[3] = ""; // empty binary + lengths[3] = 0; + formats[3] = 1; // Binary Format + + res = PQexecPrepared(conn.get(), "stmt_test_bind_5140", 4, values, lengths, formats, 0); if (PQresultStatus(res) != PGRES_COMMAND_OK) { - diag(PQerrorMessage(conn.get())); + diag("Error: %s", PQerrorMessage(conn.get())); PQclear(res); return 1; } PQclear(res); // Verify row inserted as expected - res = PQexec(conn.get(), "SELECT col1, col2 FROM reg_test_5140"); + res = PQexec(conn.get(), "SELECT col1, col2, col3, col4 FROM reg_test_5140"); if (PQresultStatus(res) != PGRES_TUPLES_OK) { - diag(PQerrorMessage(conn.get())); + diag("Error: %s", PQerrorMessage(conn.get())); PQclear(res); return 1; } char const* c1 = PQgetvalue(res, 0, 0); - bool isnull_c2 = PQgetisnull(res, 0, 2); + bool isnull_c2 = PQgetisnull(res, 0, 1); + char const* c3 = PQgetvalue(res, 0, 2); + bool isnull_c4 = PQgetisnull(res, 0, 3); + int len_c4 = PQgetlength(res, 0, 3); ok(std::string(c1) == "ABCDEFGHIJKLMN", "col1 length 14 parsed correctly"); - ok(isnull_c2, "col3 NULL parsed correctly"); + ok(isnull_c2, "col2 NULL parsed correctly"); + ok(std::string(c3) == "", "col3 empty string parsed correctly"); + ok(!isnull_c4 && len_c4 == 0, "col4 empty binary parsed correctly"); PQclear(res); From 9931fbf1d657b48da7c183f942ef033e54fe22a4 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Fri, 3 Oct 2025 16:03:02 +0500 Subject: [PATCH 6/6] Replace 0xFFFFFFFF with PGSQL_PARAM_NULL constant --- include/PgSQL_Extended_Query_Message.h | 3 +-- lib/PgSQL_Extended_Query_Message.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/PgSQL_Extended_Query_Message.h b/include/PgSQL_Extended_Query_Message.h index c453c2908..4ab3ae3f6 100644 --- a/include/PgSQL_Extended_Query_Message.h +++ b/include/PgSQL_Extended_Query_Message.h @@ -4,6 +4,7 @@ #include "proxysql.h" #include "cpp.h" +constexpr uint32_t PGSQL_PARAM_NULL = 0xFFFFFFFFu; /** * @brief Base class for handling PostgreSQL extended query messages. @@ -172,8 +173,6 @@ public: } current += sizeof(uint32_t); - constexpr uint32_t PGSQL_PARAM_NULL = 0xFFFFFFFF; - if (len != PGSQL_PARAM_NULL && len > INT32_MAX) { return false; // Malformed: length does not fit into int32_t } diff --git a/lib/PgSQL_Extended_Query_Message.cpp b/lib/PgSQL_Extended_Query_Message.cpp index 6f4a34d6e..375a6a1b0 100644 --- a/lib/PgSQL_Extended_Query_Message.cpp +++ b/lib/PgSQL_Extended_Query_Message.cpp @@ -379,7 +379,7 @@ bool PgSQL_Bind_Message::parse(PtrSize_t& pkt) { return false; } offset += sizeof(uint32_t); - if (param_value_len != 0xFFFFFFFF) { + if (param_value_len != PGSQL_PARAM_NULL) { // Ensure the parameter value size is valid if (offset + param_value_len > pkt_len) return false;