public inbox for [email protected]help / color / mirror / Atom feed
Fix unsafe PlannedStmt access in pg_stat_statements 8+ messages / 3 participants [nested] [flat]
* Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-11 08:07 Chao Li <[email protected]> 0 siblings, 2 replies; 8+ messages in thread From: Chao Li @ 2026-05-11 08:07 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, I spotted this small issue while working on [1]. In pgss_ProcessUtility(), there is this comment: ``` /* * CAUTION: do not access the *pstmt data structure again below here. * If it was a ROLLBACK or similar, that data structure may have been * freed. We must copy everything we still need into local variables, * which we did above. * * For the same reason, we can't risk restoring pstmt->queryId to its * former value, which'd otherwise be a good idea. */ ``` However, commit 3357471cf9f5e470dfed0c7919bcf31c7efaf2b9 added a new access to pstmt after that point: ``` pgss_store(queryString, saved_queryId, saved_stmt_location, saved_stmt_len, PGSS_EXEC, INSTR_TIME_GET_MILLISEC(duration), rows, &bufusage, &walusage, NULL, NULL, 0, 0, pstmt->planOrigin); ``` The attached patch fixes this by saving pstmt->planOrigin, following the same pattern already used for queryId, stmt_location, and stmt_len. [1] https://www.postgresql.org/message-id/8ED8C22D-54CD-4EC4-B53C-D39F935FA83D%40gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-11 08:11 Chao Li <[email protected]> parent: Chao Li <[email protected]> 1 sibling, 1 reply; 8+ messages in thread From: Chao Li @ 2026-05-11 08:11 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> > On May 11, 2026, at 16:07, Chao Li <[email protected]> wrote: > > Hi, > > I spotted this small issue while working on [1]. > > In pgss_ProcessUtility(), there is this comment: > ``` > /* > * CAUTION: do not access the *pstmt data structure again below here. > * If it was a ROLLBACK or similar, that data structure may have been > * freed. We must copy everything we still need into local variables, > * which we did above. > * > * For the same reason, we can't risk restoring pstmt->queryId to its > * former value, which'd otherwise be a good idea. > */ > ``` > > However, commit 3357471cf9f5e470dfed0c7919bcf31c7efaf2b9 added a new access to pstmt after that point: > ``` > pgss_store(queryString, > saved_queryId, > saved_stmt_location, > saved_stmt_len, > PGSS_EXEC, > INSTR_TIME_GET_MILLISEC(duration), > rows, > &bufusage, > &walusage, > NULL, > NULL, > 0, > 0, > pstmt->planOrigin); > ``` > > The attached patch fixes this by saving pstmt->planOrigin, following the same pattern already used for queryId, stmt_location, and stmt_len. > > [1] https://www.postgresql.org/message-id/8ED8C22D-54CD-4EC4-B53C-D39F935FA83D%40gmail.com > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > Oops! Forgot the attachment. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v1-0001-Fix-unsafe-PlannedStmt-access-in-pg_stat_statemen.patch (1.2K, 2-v1-0001-Fix-unsafe-PlannedStmt-access-in-pg_stat_statemen.patch) download | inline diff: From 05da7b75b455b8d0e30d9a77d5fbe3ca4eccae8e Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Mon, 11 May 2026 15:43:53 +0800 Subject: [PATCH v1] Fix unsafe PlannedStmt access in pg_stat_statements Author: Chao Li <[email protected]> --- contrib/pg_stat_statements/pg_stat_statements.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 95a5411a39d..a2d3ab770cc 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1099,6 +1099,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, int64 saved_queryId = pstmt->queryId; int saved_stmt_location = pstmt->stmt_location; int saved_stmt_len = pstmt->stmt_len; + PlannedStmtOrigin saved_planOrigin = pstmt->planOrigin; bool enabled = pgss_track_utility && pgss_enabled(nesting_level); /* @@ -1210,7 +1211,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, NULL, 0, 0, - pstmt->planOrigin); + saved_planOrigin); } else { -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-11 08:11 Michael Paquier <[email protected]> parent: Chao Li <[email protected]> 1 sibling, 0 replies; 8+ messages in thread From: Michael Paquier @ 2026-05-11 08:11 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, May 11, 2026 at 04:07:29PM +0800, Chao Li wrote: > However, commit 3357471cf9f5e470dfed0c7919bcf31c7efaf2b9 added a new > access to pstmt after that point: > > The attached patch fixes this by saving pstmt->planOrigin, following > the same pattern already used for queryId, stmt_location, and > stmt_len. That would be my business. Missing a patch attached perhaps? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-12 03:30 Michael Paquier <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Michael Paquier @ 2026-05-12 03:30 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, May 11, 2026 at 04:11:41PM +0800, Chao Li wrote: > On May 11, 2026, at 16:07, Chao Li <[email protected]> wrote: >> In pgss_ProcessUtility(), there is this comment: >> ``` >> /* >> * CAUTION: do not access the *pstmt data structure again below here. >> * If it was a ROLLBACK or similar, that data structure may have been >> * freed. We must copy everything we still need into local variables, >> * which we did above. >> * >> * For the same reason, we can't risk restoring pstmt->queryId to its >> * former value, which'd otherwise be a good idea. >> */ >> ``` >> >> The attached patch fixes this by saving pstmt->planOrigin, >> following the same pattern already used for queryId, stmt_location, >> and stmt_len. Yeah, you are right. This code should save the planOrigin but it does not do so. This is also pointing at a second issue that is not addressed by your patch: there is no coverage for the assumption of this comment, or we would have known about this issue when 3357471cf9f5 was committed. Running the regression tests of PGSS with valgrind enabled leads to no reports. And that's where things got hairy on my side because it took me a few hours before finding a case where I was able to get the following report, where a PlannedStmt can get freed while we are in the hook path: ==28059== Invalid read of size 4 ==28059== at 0x165E42DF: pgss_ProcessUtility (pg_stat_statements.c:1200) ==28059== by 0x158AFCD: ProcessUtility (utility.c:524) ==28059== by 0x1587F3B: PortalRunUtility (pquery.c:1149) ==28059== by 0x15877E9: FillPortalStore (pquery.c:1022) This has come down to the extended protocol and a ROLLBACK inside a CALL procedure. The internal ROLLBACK is able to free the PlannedStmt in pgss_ProcessUtility() in this case, after two executions. > Oops! Forgot the attachment. No problem. All that said, I am attaching an updated version of the patch, for posterity. I'll apply that in a bit with your fix and the test on HEAD, after more checks. -- Michael From 19281d8708facd17cd65a9ab07dd7b775496a5c9 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 12 May 2026 12:24:52 +0900 Subject: [PATCH] pg_stat_statements: Fix potential use-after-free of PlannedStmt Blahblahblah. Author: Chao Li Co-authored-by: Michael Paquier Discussion: https://postgr.es/m/ --- .../pg_stat_statements/expected/plancache.out | 38 +++++++++++++++++++ .../pg_stat_statements/pg_stat_statements.c | 3 +- contrib/pg_stat_statements/sql/plancache.sql | 19 ++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/expected/plancache.out b/contrib/pg_stat_statements/expected/plancache.out index 32bf913b2861..d0796d5693c0 100644 --- a/contrib/pg_stat_statements/expected/plancache.out +++ b/contrib/pg_stat_statements/expected/plancache.out @@ -216,6 +216,44 @@ SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, query FROM pg_sta RESET pg_stat_statements.track; -- +-- Procedure with internal ROLLBACK and the extended query protocol. +-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal +-- ROLLBACK. +-- +CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$ +BEGIN + ROLLBACK; +END; +$$ LANGUAGE plpgsql; +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +CALL rollback_proc($1) \parse stmt_rollback +\bind_named stmt_rollback 1 \g + a +--- + 1 +(1 row) + +\bind_named stmt_rollback 2 \g + a +--- + 2 +(1 row) + +SELECT calls, query FROM pg_stat_statements + WHERE query LIKE '%rollback_proc%' + ORDER BY query COLLATE "C"; + calls | query +-------+------------------------ + 2 | CALL rollback_proc($1) +(1 row) + +DROP PROCEDURE rollback_proc; +-- -- Cleanup -- DROP FUNCTION select_one_func(int); diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 95a5411a39d9..a2d3ab770cc6 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1099,6 +1099,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, int64 saved_queryId = pstmt->queryId; int saved_stmt_location = pstmt->stmt_location; int saved_stmt_len = pstmt->stmt_len; + PlannedStmtOrigin saved_planOrigin = pstmt->planOrigin; bool enabled = pgss_track_utility && pgss_enabled(nesting_level); /* @@ -1210,7 +1211,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, NULL, 0, 0, - pstmt->planOrigin); + saved_planOrigin); } else { diff --git a/contrib/pg_stat_statements/sql/plancache.sql b/contrib/pg_stat_statements/sql/plancache.sql index 160ced7add36..948d3e985180 100644 --- a/contrib/pg_stat_statements/sql/plancache.sql +++ b/contrib/pg_stat_statements/sql/plancache.sql @@ -87,6 +87,25 @@ SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, query FROM pg_sta RESET pg_stat_statements.track; +-- +-- Procedure with internal ROLLBACK and the extended query protocol. +-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal +-- ROLLBACK. +-- +CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$ +BEGIN + ROLLBACK; +END; +$$ LANGUAGE plpgsql; +SELECT pg_stat_statements_reset() IS NOT NULL AS t; +CALL rollback_proc($1) \parse stmt_rollback +\bind_named stmt_rollback 1 \g +\bind_named stmt_rollback 2 \g +SELECT calls, query FROM pg_stat_statements + WHERE query LIKE '%rollback_proc%' + ORDER BY query COLLATE "C"; +DROP PROCEDURE rollback_proc; + -- -- Cleanup -- -- 2.54.0 Attachments: [text/plain] 0001-pg_stat_statements-Fix-potential-use-after-free-of-P.patch (3.5K, 2-0001-pg_stat_statements-Fix-potential-use-after-free-of-P.patch) download | inline diff: From 19281d8708facd17cd65a9ab07dd7b775496a5c9 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 12 May 2026 12:24:52 +0900 Subject: [PATCH] pg_stat_statements: Fix potential use-after-free of PlannedStmt Blahblahblah. Author: Chao Li Co-authored-by: Michael Paquier Discussion: https://postgr.es/m/ --- .../pg_stat_statements/expected/plancache.out | 38 +++++++++++++++++++ .../pg_stat_statements/pg_stat_statements.c | 3 +- contrib/pg_stat_statements/sql/plancache.sql | 19 ++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/expected/plancache.out b/contrib/pg_stat_statements/expected/plancache.out index 32bf913b2861..d0796d5693c0 100644 --- a/contrib/pg_stat_statements/expected/plancache.out +++ b/contrib/pg_stat_statements/expected/plancache.out @@ -216,6 +216,44 @@ SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, query FROM pg_sta RESET pg_stat_statements.track; -- +-- Procedure with internal ROLLBACK and the extended query protocol. +-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal +-- ROLLBACK. +-- +CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$ +BEGIN + ROLLBACK; +END; +$$ LANGUAGE plpgsql; +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +CALL rollback_proc($1) \parse stmt_rollback +\bind_named stmt_rollback 1 \g + a +--- + 1 +(1 row) + +\bind_named stmt_rollback 2 \g + a +--- + 2 +(1 row) + +SELECT calls, query FROM pg_stat_statements + WHERE query LIKE '%rollback_proc%' + ORDER BY query COLLATE "C"; + calls | query +-------+------------------------ + 2 | CALL rollback_proc($1) +(1 row) + +DROP PROCEDURE rollback_proc; +-- -- Cleanup -- DROP FUNCTION select_one_func(int); diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 95a5411a39d9..a2d3ab770cc6 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1099,6 +1099,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, int64 saved_queryId = pstmt->queryId; int saved_stmt_location = pstmt->stmt_location; int saved_stmt_len = pstmt->stmt_len; + PlannedStmtOrigin saved_planOrigin = pstmt->planOrigin; bool enabled = pgss_track_utility && pgss_enabled(nesting_level); /* @@ -1210,7 +1211,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, NULL, 0, 0, - pstmt->planOrigin); + saved_planOrigin); } else { diff --git a/contrib/pg_stat_statements/sql/plancache.sql b/contrib/pg_stat_statements/sql/plancache.sql index 160ced7add36..948d3e985180 100644 --- a/contrib/pg_stat_statements/sql/plancache.sql +++ b/contrib/pg_stat_statements/sql/plancache.sql @@ -87,6 +87,25 @@ SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, query FROM pg_sta RESET pg_stat_statements.track; +-- +-- Procedure with internal ROLLBACK and the extended query protocol. +-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal +-- ROLLBACK. +-- +CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$ +BEGIN + ROLLBACK; +END; +$$ LANGUAGE plpgsql; +SELECT pg_stat_statements_reset() IS NOT NULL AS t; +CALL rollback_proc($1) \parse stmt_rollback +\bind_named stmt_rollback 1 \g +\bind_named stmt_rollback 2 \g +SELECT calls, query FROM pg_stat_statements + WHERE query LIKE '%rollback_proc%' + ORDER BY query COLLATE "C"; +DROP PROCEDURE rollback_proc; + -- -- Cleanup -- -- 2.54.0 [application/pgp-signature] signature.asc (833B, 3-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-12 09:00 Andres Freund <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Andres Freund @ 2026-05-12 09:00 UTC (permalink / raw) To: [email protected]; Michael Paquier <[email protected]>; Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi, On May 12, 2026 5:30:53 AM GMT+02:00, Michael Paquier <[email protected]> wrote: >On Mon, May 11, 2026 at 04:11:41PM +0800, Chao Li wrote: >> On May 11, 2026, at 16:07, Chao Li <[email protected]> wrote: >>> In pgss_ProcessUtility(), there is this comment: >>> ``` >>> /* >>> * CAUTION: do not access the *pstmt data structure again below here. >>> * If it was a ROLLBACK or similar, that data structure may have been >>> * freed. We must copy everything we still need into local variables, >>> * which we did above. >>> * >>> * For the same reason, we can't risk restoring pstmt->queryId to its >>> * former value, which'd otherwise be a good idea. >>> */ >>> ``` >>> >>> The attached patch fixes this by saving pstmt->planOrigin, >>> following the same pattern already used for queryId, stmt_location, >>> and stmt_len. > >Yeah, you are right. This code should save the planOrigin but it does >not do so. Seems like the code should make this clearer, by simply unsetting pstmt at the point it becomes unsafe to use? Andres ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-13 04:26 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Michael Paquier @ 2026-05-13 04:26 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: [email protected]; Chao Li <[email protected]> On Tue, May 12, 2026 at 11:00:16AM +0200, Andres Freund wrote: > Seems like the code should make this clearer, by simply unsetting > pstmt at the point it becomes unsafe to use? Sure, we could do that as well and crash hard if something decides to do the same mistake. Like the simple patch attached for example? I am not sure if we need to update the comment. It's pretty clear what the intention is, at least to me. -- Michael diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index a2d3ab770cc6..92315627916d 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1175,6 +1175,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, * For the same reason, we can't risk restoring pstmt->queryId to its * former value, which'd otherwise be a good idea. */ + pstmt = NULL; INSTR_TIME_SET_CURRENT(duration); INSTR_TIME_SUBTRACT(duration, start); Attachments: [text/plain] pgss-pstmt.patch (577B, 2-pgss-pstmt.patch) download | inline diff: diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index a2d3ab770cc6..92315627916d 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1175,6 +1175,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, * For the same reason, we can't risk restoring pstmt->queryId to its * former value, which'd otherwise be a good idea. */ + pstmt = NULL; INSTR_TIME_SET_CURRENT(duration); INSTR_TIME_SUBTRACT(duration, start); [application/pgp-signature] signature.asc (833B, 3-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-13 05:31 Chao Li <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Chao Li @ 2026-05-13 05:31 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected] > On May 13, 2026, at 12:26, Michael Paquier <[email protected]> wrote: > > On Tue, May 12, 2026 at 11:00:16AM +0200, Andres Freund wrote: >> Seems like the code should make this clearer, by simply unsetting >> pstmt at the point it becomes unsafe to use? > > Sure, we could do that as well and crash hard if something decides to > do the same mistake. Like the simple patch attached for example? I > am not sure if we need to update the comment. It's pretty clear what > the intention is, at least to me. > -- > Michael > <pgss-pstmt.patch> Yeah, this is the exactly same change I was about to reply. And I agree the current comment is clear enough, no more needed to add. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix unsafe PlannedStmt access in pg_stat_statements @ 2026-05-13 06:22 Michael Paquier <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Michael Paquier @ 2026-05-13 06:22 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected] On Wed, May 13, 2026 at 01:31:01PM +0800, Chao Li wrote: > Yeah, this is the exactly same change I was about to reply. And I > agree the current comment is clear enough, no more needed to add. Okay. Let's just do that, then. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-05-13 06:22 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-11 08:07 Fix unsafe PlannedStmt access in pg_stat_statements Chao Li <[email protected]> 2026-05-11 08:11 ` Chao Li <[email protected]> 2026-05-12 03:30 ` Michael Paquier <[email protected]> 2026-05-12 09:00 ` Andres Freund <[email protected]> 2026-05-13 04:26 ` Michael Paquier <[email protected]> 2026-05-13 05:31 ` Chao Li <[email protected]> 2026-05-13 06:22 ` Michael Paquier <[email protected]> 2026-05-11 08:11 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox