From ee40e89c960214a2f1ad32d0d4a713ee6be525a1 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Sep 2026 21:26:15 +0000 Subject: [PATCH v5 4/5] Fix crashes and missing checks in _pq_.cursor Bind and Execute handling Replan a SCROLL portal when its cached plan is parallel or cannot scan backwards, as PerformCursorOpen does. Reject WITH HOLD unless the portal holds a single SELECT. Reject SCROLL and WITH HOLD with FOR UPDATE/SHARE. Reject a fetch count of INT64_MIN. Check nParams in the libpq cursor Bind functions. --- doc/src/sgml/protocol.sgml | 13 ++++- src/backend/tcop/postgres.c | 100 ++++++++++++++++++++++++++++++--- src/interfaces/libpq/fe-exec.c | 7 +++ 3 files changed, 110 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 4ee69d51fe3..4f366d60763 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -353,10 +353,16 @@ 0x0004 — WITH HOLD SCROLL and NO SCROLL are mutually exclusive. - WITH HOLD is not permitted on unnamed portals. + WITH HOLD is not permitted on unnamed portals, and only for a + single SELECT, as for + DECLARE. + Neither SCROLL nor WITH HOLD is permitted with + FOR UPDATE or FOR SHARE. All other bits are reserved and must be zero. A portal is scrollable only if SCROLL is requested, so NO SCROLL is - accepted but never necessary. A value of 0 requests no cursor options + accepted but never necessary. SCROLL may cause the + statement to be replanned when its cached plan cannot be read + backwards. A value of 0 requests no cursor options at all, and creates exactly the portal that a Bind message without this extension would create. @@ -5249,7 +5255,8 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;" Number of rows to fetch, interpreted as the count of the FETCH command of the same direction would be. The largest positive value - (0x7FFFFFFFFFFFFFFF) means ALL. + (0x7FFFFFFFFFFFFFFF) means ALL; + the most negative value is not allowed. This field must be zero if the fetch flags are 0. diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index cf43acd54b3..6fcf971a64a 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -41,6 +41,7 @@ #include "commands/prepare.h" #include "commands/repack.h" #include "common/pg_prng.h" +#include "executor/executor.h" #include "jit/jit.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -1671,6 +1672,7 @@ exec_bind_message(StringInfo input_message) int16 *rformats = NULL; CachedPlanSource *psrc; CachedPlan *cplan; + List *stmt_list; Portal portal; char *query_string; char *saved_stmt_name; @@ -1682,6 +1684,7 @@ exec_bind_message(StringInfo input_message) ParamsErrorCbData params_data; ErrorContextCallback params_errcxt; ListCell *lc; + int bind_ext_flags = 0; /* Get the fixed part of the message */ portal_name = pq_getmsgstring(input_message); @@ -2059,8 +2062,6 @@ exec_bind_message(StringInfo input_message) */ if (MyProcPort != NULL && MyProcPort->protocol_cursor_enabled) { - int bind_ext_flags; - bind_ext_flags = pq_getmsgint(input_message, 4); /* Reject any bits we don't recognize */ @@ -2109,18 +2110,96 @@ exec_bind_message(StringInfo input_message) * assigned to the Portal, so it will be released at portal destruction. */ cplan = GetCachedPlan(psrc, params, NULL, NULL); + stmt_list = cplan->stmt_list; + + /* + * DECLARE CURSOR's restrictions on SCROLL and WITH HOLD depend on the + * planned statement, so check them here. Release the plan before + * erroring out; nothing else would. + */ + if (bind_ext_flags & (PQ_BIND_CURSOR_SCROLL | PQ_BIND_CURSOR_HOLD)) + { + PortalStrategy strategy = ChoosePortalStrategy(cplan->stmt_list); + + /* PersistHoldablePortal can only cope with a single SELECT */ + if ((bind_ext_flags & PQ_BIND_CURSOR_HOLD) && + strategy != PORTAL_ONE_SELECT) + { + ReleaseCachedPlan(cplan, NULL); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WITH HOLD cursor option is only allowed for a SELECT statement"))); + } + + /* FOR UPDATE/SHARE, as in transformDeclareCursorStmt */ + if (strategy == PORTAL_ONE_SELECT && + linitial_node(PlannedStmt, cplan->stmt_list)->rowMarks != NIL) + { + if (bind_ext_flags & PQ_BIND_CURSOR_HOLD) + { + ReleaseCachedPlan(cplan, NULL); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WITH HOLD cursor option is not supported with FOR UPDATE/SHARE"), + errdetail("Holdable cursors must be READ ONLY."))); + } + if (bind_ext_flags & PQ_BIND_CURSOR_SCROLL) + { + ReleaseCachedPlan(cplan, NULL); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("SCROLL cursor option is not supported with FOR UPDATE/SHARE"), + errdetail("Scrollable cursors must be READ ONLY."))); + } + } + + /* + * SCROLL starts the executor with EXEC_FLAG_BACKWARD, which only a + * plan built with CURSOR_OPT_SCROLL is sure to support: the planner + * then materializes anything that cannot scan backwards, and without + * CURSOR_OPT_PARALLEL_OK there is no Gather. The cached plan was + * built without either, so if it is not usable as is, plan afresh as + * PerformCursorOpen does and give the portal the plan outright. + */ + if ((bind_ext_flags & PQ_BIND_CURSOR_SCROLL) && + strategy == PORTAL_ONE_SELECT) + { + PlannedStmt *pstmt = linitial_node(PlannedStmt, cplan->stmt_list); + + if (pstmt->parallelModeNeeded || + !ExecSupportsBackwardScan(pstmt->planTree)) + { + int cursor_options; + + ReleaseCachedPlan(cplan, NULL); + cplan = NULL; + + cursor_options = (psrc->cursor_options & ~CURSOR_OPT_PARALLEL_OK) | + CURSOR_OPT_SCROLL; + stmt_list = pg_plan_queries(copyObject(psrc->query_list), + psrc->query_string, + cursor_options, params); + + /* the portal owns it, so it goes in its context */ + oldContext = MemoryContextSwitchTo(portal->portalContext); + stmt_list = copyObject(stmt_list); + MemoryContextSwitchTo(oldContext); + } + } + } /* * Now we can define the portal. * * DO NOT put any code that could possibly throw an error between the - * above GetCachedPlan call and here. + * above GetCachedPlan call and here, except the checks above, which + * release the plan first. */ PortalDefineQuery(portal, saved_stmt_name, query_string, psrc->commandTag, - cplan->stmt_list, + stmt_list, cplan); /* Portal is defined, set the plan ID based on its contents. */ @@ -2213,15 +2292,22 @@ fetch_count_wire_to_long(int64 count) if (count == PQ_FETCH_ALL) return FETCH_ALL; /* == LONG_MAX */ + /* DoPortalRunFetch negates negative counts; reject the one that overflows */ + if (count == PG_INT64_MIN) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("fetch count out of range"))); + /* * The wire count is a full 64-bit integer, but "long" is only 32 bits * where SIZEOF_LONG < 8 (LLP64 Windows, ILP32 platforms). There, a value * that does not fit would be silently truncated by the cast below, so - * reject it instead. Where "long" is 64 bits this test is always false, - * so compile it out rather than emit a tautological comparison. + * reject it instead, along with LONG_MIN itself for the reason above. + * Where "long" is 64 bits this test is always false, so compile it out + * rather than emit a tautological comparison. */ #if SIZEOF_LONG < 8 - if (count > LONG_MAX || count < LONG_MIN) + if (count > LONG_MAX || count <= LONG_MIN) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("fetch count out of range for this platform"))); diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 98dd831de4e..4e434798e7b 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -1968,6 +1968,13 @@ PQsendBindGuts(PGconn *conn, return 0; } + if (nParams < 0 || nParams > PQ_QUERY_PARAM_MAX_LIMIT) + { + libpq_append_conn_error(conn, "number of parameters must be between 0 and %d", + PQ_QUERY_PARAM_MAX_LIMIT); + return 0; + } + if (cursorOptions != 0 && !conn->protocol_cursor_enabled) { libpq_append_conn_error(conn, -- 2.17.1