Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_stat_statements: Set PlannedStmt to NULL after nested utility execution

  2. pg_stat_statements: Fix potential use-after-free of PlannedStmt

  3. pg_stat_statements: Add counters for generic and custom plans

  1. Fix unsafe PlannedStmt access in pg_stat_statements

    Chao Li <li.evan.chao@gmail.com> — 2026-05-11T08:07:29Z

    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/

  2. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Chao Li <li.evan.chao@gmail.com> — 2026-05-11T08:11:41Z

    
> On May 11, 2026, at 16:07, Chao Li <li.evan.chao@gmail.com> 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/

  3. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Michael Paquier <michael@paquier.xyz> — 2026-05-11T08:11:45Z

    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

  4. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Michael Paquier <michael@paquier.xyz> — 2026-05-12T03:30:53Z

    On Mon, May 11, 2026 at 04:11:41PM +0800, Chao Li wrote:
> On May 11, 2026, at 16:07, Chao Li <li.evan.chao@gmail.com> 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

  5. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Andres Freund <andres@anarazel.de> — 2026-05-12T09:00:16Z

    Hi, 

On May 12, 2026 5:30:53 AM GMT+02:00, Michael Paquier <michael@paquier.xyz> wrote:
>On Mon, May 11, 2026 at 04:11:41PM +0800, Chao Li wrote:
>> On May 11, 2026, at 16:07, Chao Li <li.evan.chao@gmail.com> 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

  6. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Michael Paquier <michael@paquier.xyz> — 2026-05-13T04:26:36Z

    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

  7. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Chao Li <li.evan.chao@gmail.com> — 2026-05-13T05:31:01Z

    
> On May 13, 2026, at 12:26, Michael Paquier <michael@paquier.xyz> 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/

  8. Re: Fix unsafe PlannedStmt access in pg_stat_statements

    Michael Paquier <michael@paquier.xyz> — 2026-05-13T06:22:29Z

    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