Conversation
Port fixes from apache#2029 back to REL_2_STABLE branch Five issues from review of the PG16 port. Three share a root cause worth stating plainly: the collection path runs inside PG_TRY() from a signal handler, mixes C and C++ frames, and dismisses its own errors. Every non-local exit that skips cleanup therefore leaves damage behind in a backend that then keeps serving. 1. Leaked interrupt holdoff -- longjmp over RESUME_INTERRUPTS SendQueryState() holds interrupts for the whole collection, and build_plan_doc() took a second, nested hold. ExplainPrintPlan() can raise an error, and the longjmp out of it skips the inner RESUME_INTERRUPTS(); the PG_CATCH path that calls elog_dismiss(WARNING) then resumes only the caller's hold. InterruptHoldoffCount stays at 1 for the rest of the session and CHECK_FOR_INTERRUPTS() becomes a no-op, so the backend can no longer be cancelled or terminated. The re-throw path self-heals, because transaction abort resets the count -- the dismiss path, which is the common one here, does not. build_plan_doc() has exactly one caller, so drop the redundant pair rather than wrap it in PG_FINALLY, and assert the precondition so the coupling is checkable instead of implicit. 2. C++ exceptions escaping extern "C" gpsc_qs_sync_config(), gpsc_emit_node_batch() and gpsc_emit_query_plan() are reached from C on the signal path with no exception boundary. Config::sync(), protobuf construction and serialization, and UDSConnector::report_extended() -- which has no try/catch of its own and uses std::string plus an RAII socket guard -- can all throw. An exception crossing an extern "C" frame is undefined behaviour and in practice calls std::terminate(), taking the backend down. Add qs_emit_guard() and route all three entry points through it. cpp_call() in hook_wrappers.cpp cannot be reused as-is: it is a file-static template shaped for member-function pointers. It also differs deliberately in what it does once it has caught something. cpp_call() raises a PostgreSQL error, but an ereport(ERROR) longjmp from here would unwind C++ frames without running their destructors -- leaking the socket guard's fd -- and abandon the rest of the snapshot. This is best-effort telemetry taken while somebody else's query is mid-flight, so report at WARNING and return; a missing batch is just a missed sample. 3. Missing STRICT on the trace-bearing functions pg_query_state(), cbdb_mpp_query_state() and pg_query_state_backends() were declared without STRICT while every other function in both scripts has it. PG_GETARG_BYTEA_P() and PG_GETARG_ARRAYTYPE_P() do not consult isnull; they detoast a zero Datum, so passing NULL dereferences a null pointer instead of raising a controlled error. Mark all three STRICT in the fresh-install and the 1.1-to-1.2 script. 4. Unauthorized polling through the PUBLIC QE entry point cbdb_mpp_query_state() is granted to PUBLIC, carries no EXECUTE ON clause so it also runs on the coordinator, and selects targets by comparing the caller-supplied segid against GpIdentity.segindex -- which is -1 on the QD. An unprivileged session could therefore name (-1, victim_pid) directly and have another role's live plan collected and pushed to the UDS sink, bypassing the superuser-or-owner gate that pg_query_state() applies on the coordinator. It cannot simply be revoked, because CdbDispatchCommand() runs it on the QEs as the session user. Apply the same per-target check inside the function instead. This cannot reject a legitimate dispatch: the coordinator has already authorized the caller, and a query's QEs run under the same role as its coordinator backend. 5. Pending custom-signal flags not volatile sig_atomic_t CustomSignalPendings is written by procsignal_sigusr1_handler() and read by CheckAndHandleCustomSignals() in normal code -- the same contract as pss_signalFlags in the same file, which is volatile sig_atomic_t. As a plain bool array the compiler may keep a stale copy across the read and silently drop notifications. CustomSignalProcessing (a recursion guard) and CustomInterruptHandlers (set once at _PG_init) are never touched from the handler and stay ordinary variables; say so in a comment.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (4)
planstate->worker_instrumentbefore dereferencing/using it (it’s… · NewGpscNodeSampleis part of a binary/shared-memory interface, storingnode_typeas a Cenum… · Newnfiltered, but the printed value is… · New
What changed in this PR
Backports multiple hardening fixes to pg_query_state for REL_2_STABLE, focusing on signal-path robustness, privilege enforcement, and making plan node typing stable across major PostgreSQL versions.
Changes:
- Prevent backend corruption from non-local exits by removing nested interrupt holdoffs and adding C++ exception guards on extern "C" entry points.
- Enforce per-target ownership/superuser checks in the QE entry point to prevent unauthorized query-state polling.
- Introduce a stable, protocol-defined plan node type (decoupled from
NodeTag) and plumb it through sampling + protobuf.
| File | Description |
|---|---|
| src/backend/storage/ipc/procsignal.c | Makes custom-signal pending flags safe for signal handler communication. |
| src/backend/commands/explain.c | Adjusts worker-detail gating and filtered-row reporting to avoid unsafe states/div-by-zero. |
| gpcontrib/gp_stats_collector/src/pg_query_state/signal_handler.c | Adds stable node-type mapping and walker support for T_Sequence; removes nested interrupt hold. |
| gpcontrib/gp_stats_collector/src/pg_query_state/qs_types.h | Adds protocol-stable QsPlanNodeType and replaces raw NodeTag storage in samples. |
| gpcontrib/gp_stats_collector/src/pg_query_state/pg_query_state.c | Adds per-target permission checks to prevent unauthorized polling via PUBLIC entry point. |
| gpcontrib/gp_stats_collector/src/PlanNodeEmitter.cpp | Adds exception boundary for extern "C" entry points; maps QsPlanNodeType to protobuf enum. |
| gpcontrib/gp_stats_collector/protos/yagpcc_set_per_node.proto | Changes per-node batch node_type to protocol PlanNodeType (not raw NodeTag). |
| gpcontrib/gp_stats_collector/protos/yagpcc_plan.proto | Defines PlanNodeType enum and documents remaining raw-NodeTag field as unused/unsafe. |
| gpcontrib/gp_stats_collector/gp_stats_collector--1.2.sql | Marks trace-bearing functions STRICT to avoid NULL-datum crashes. |
| gpcontrib/gp_stats_collector/gp_stats_collector--1.1--1.2.sql | Same STRICT fix for upgrade path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2978
to
2979
| if (es->workers_state && (es->buffers || es->wal) && es->verbose | ||
| && !es->runtime) |
Comment on lines
+166
to
+167
| QsPlanNodeType node_type; /* qs_map_node_type(nodeTag(plan)); a | ||
| * protocol value, not a raw NodeTag */ |
Comment on lines
+4515
to
+4535
|
|
||
| if (which == 2) | ||
| nfiltered = ((nloops > 0) ? planstate->instrument->nfiltered2 / nloops : 0); | ||
| nfiltered = planstate->instrument->nfiltered2; | ||
| else | ||
| nfiltered = ((nloops > 0) ? planstate->instrument->nfiltered1 / nloops : 0); | ||
| nfiltered = planstate->instrument->nfiltered1; | ||
| nloops = planstate->instrument->nloops; | ||
|
|
||
| /* In text mode, suppress zero counts; they're not interesting enough */ | ||
| /* | ||
| * In text mode, suppress zero counts; they're not interesting enough. | ||
| * | ||
| * The nloops == 0 case is what runtime mode hits for the whole of the first | ||
| * loop, so the counters cannot be averaged there; report 0 rather than | ||
| * dividing by zero. | ||
| */ | ||
| if (nfiltered > 0 || es->format != EXPLAIN_FORMAT_TEXT) | ||
| ExplainPropertyFloat(qlabel, NULL, nfiltered, 0, es); | ||
| { | ||
| if (nloops > 0) | ||
| ExplainPropertyFloat(qlabel, NULL, nfiltered / nloops, 0, es); | ||
| else | ||
| ExplainPropertyFloat(qlabel, NULL, 0.0, 0, es); | ||
| } |
Comment on lines
+315
to
+317
| default: | ||
| elog(DEBUG1, "pg_query_state: unmapped plan NodeTag %d", (int) tag); | ||
| return QS_PLAN_NODE_TYPE_UNSPECIFIED; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port fixes from #2029 back to REL_2_STABLE branch
Five issues from review of the PG16 port. Three share a root cause worth stating plainly: the collection path runs inside PG_TRY() from a signal handler, mixes C and C++ frames, and dismisses its own errors. Every non-local exit that skips cleanup therefore leaves damage behind in a backend that then keeps serving.
SendQueryState() holds interrupts for the whole collection, and build_plan_doc() took a second, nested hold. ExplainPrintPlan() can raise an error, and the longjmp out of it skips the inner RESUME_INTERRUPTS(); the PG_CATCH path that calls elog_dismiss(WARNING) then resumes only the caller's hold. InterruptHoldoffCount stays at 1 for the rest of the session and CHECK_FOR_INTERRUPTS() becomes a no-op, so the backend can no longer be cancelled or terminated. The re-throw path self-heals, because transaction abort resets the count -- the dismiss path, which is the common one here, does not.
build_plan_doc() has exactly one caller, so drop the redundant pair rather than wrap it in PG_FINALLY, and assert the precondition so the coupling is checkable instead of implicit.
gpsc_qs_sync_config(), gpsc_emit_node_batch() and gpsc_emit_query_plan() are reached from C on the signal path with no exception boundary. Config::sync(), protobuf construction and serialization, and UDSConnector::report_extended() -- which has no try/catch of its own and uses std::string plus an RAII socket guard -- can all throw. An exception crossing an extern "C" frame is undefined behaviour and in practice calls std::terminate(), taking the backend down.
Add qs_emit_guard() and route all three entry points through it. cpp_call() in hook_wrappers.cpp cannot be reused as-is: it is a file-static template shaped for member-function pointers. It also differs deliberately in what it does once it has caught something. cpp_call() raises a PostgreSQL error, but an ereport(ERROR) longjmp from here would unwind C++ frames without running their destructors -- leaking the socket guard's fd -- and abandon the rest of the snapshot. This is best-effort telemetry taken while somebody else's query is mid-flight, so report at WARNING and return; a missing batch is just a missed sample.
pg_query_state(), cbdb_mpp_query_state() and pg_query_state_backends() were declared without STRICT while every other function in both scripts has it. PG_GETARG_BYTEA_P() and PG_GETARG_ARRAYTYPE_P() do not consult isnull; they detoast a zero Datum, so passing NULL dereferences a null pointer instead of raising a controlled error. Mark all three STRICT in the fresh-install and the 1.1-to-1.2 script.
cbdb_mpp_query_state() is granted to PUBLIC, carries no EXECUTE ON clause so it also runs on the coordinator, and selects targets by comparing the caller-supplied segid against GpIdentity.segindex -- which is -1 on the QD. An unprivileged session could therefore name (-1, victim_pid) directly and have another role's live plan collected and pushed to the UDS sink, bypassing the superuser-or-owner gate that pg_query_state() applies on the coordinator.
It cannot simply be revoked, because CdbDispatchCommand() runs it on the QEs as the session user. Apply the same per-target check inside the function instead. This cannot reject a legitimate dispatch: the coordinator has already authorized the caller, and a query's QEs run under the same role as its coordinator backend.
CustomSignalPendings is written by procsignal_sigusr1_handler() and read by CheckAndHandleCustomSignals() in normal code -- the same contract as pss_signalFlags in the same file, which is volatile sig_atomic_t. As a plain bool array the compiler may keep a stale copy across the read and silently drop notifications. CustomSignalProcessing (a recursion guard) and CustomInterruptHandlers (set once at _PG_init) are never touched from the handler and stay ordinary variables; say so in a comment.