public inbox for [email protected]
help / color / mirror / Atom feedFrom: anupam MEDIRATTA <[email protected]>
To: [email protected]
Subject: [PATCH] Defensive hardening: Replace sprintf with snprintf in pg_stat_statements
Date: Mon, 25 May 2026 15:59:28 +0530
Message-ID: <CAEFUcvonaJe1fNZfGCEq3U8CKWFjC=W1JYSfUD7m_2Ox4_Cv-g@mail.gmail.com> (raw)
Hello,
I'm submitting a defensive hardening patch for contrib/pg_stat_statements
that replaces sprintf with snprintf when generating normalised query
placeholders.
## Background
While reviewing pg_stat_statements normalisation code, I noticed that
generate_normalized_query() uses unbounded sprintf to format parameter
placeholders ($1, $2, etc.) into the normalised query buffer:
n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d%s",
num_constants_replaced + 1 +
jstate->highest_extern_param_id,
locs[i].squashed ? " /*, ... */" : "");
## Analysis
The current buffer sizing logic appears to account for worst-case growth.
Each constant contributes at least one byte, while each generated
placeholder is bounded to at most 11 bytes ("$2147483647" for INT_MAX
parameter IDs, plus the optional squash comment). Given this invariant, an
actual overflow is unlikely under normal circumstances.
However, the use of unbounded sprintf makes the safety of this code
dependent on reasoning about buffer sizing logic elsewhere in the function,
rather than being locally verifiable.
## Proposed Change
This patch replaces sprintf with snprintf, making the write bound explicit
at the formatting site:
n_quer_loc += snprintf(norm_query + n_quer_loc,
norm_query_buflen - n_quer_loc + 1,
"$%d%s",
num_constants_replaced + 1 +
jstate->highest_extern_param_id,
locs[i].squashed ? " /*, ... */" : "");
This is defensive hardening only - I am not claiming this fixes a
demonstrated overflow vulnerability. The change simply makes the local
write bound explicit and protects against potential future modifications to
the buffer sizing logic.
## Additional Safety Improvements
The patch also includes related NULL pointer safety improvements in the
same file:
- Initialise qbuffer to NULL in gc_qtexts()
- Set qbuffer to NULL after pfree() in gc_qtexts() (both success and error
paths)
- Set qbuffer to NULL after pfree() in pgss_shmem_shutdown() error path
These follow defensive programming practices and guard against potential
use-after-free scenarios.
## Testing
Built and tested on macOS (Darwin 25.4.0):
- pg_stat_statements module compiles cleanly without errors or warnings
- No whitespace issues (git diff --check passed)
- No performance impact expected (same code path, just bounded write)
- Total changes: 13 insertions, 4 deletions in one file
## Context
This improvement was suggested by a downstream PostgreSQL-based project
(Apache Cloudberry) maintainer, who recommended that defensive hardening
belongs in PostgreSQL upstream rather than being maintained as a downstream
patch. Reference:
https://github.com/apache/cloudberry/pull/1744#issuecomment-4458061490
Patch attached (pg_stat_statements-sprintf-snprintf-v1.patch). I'm happy to
address any feedback or concerns.
Best Regards,
Anupam
Attachments:
[application/octet-stream] pg_stat_statements-sprintf-snprintf-v1.patch (3.0K, 3-pg_stat_statements-sprintf-snprintf-v1.patch)
download | inline diff:
From 5db105888cfe49527238183903c859179701e6b5 Mon Sep 17 00:00:00 2001
From: OrbisAI Security <[email protected]>
Date: Mon, 25 May 2026 15:18:08 +0530
Subject: [PATCH v1] Defensive hardening: Replace sprintf with snprintf in
pg_stat_statements
Replace unbounded sprintf with snprintf when generating normalized query
parameter placeholders ($1, $2, etc.) in pg_stat_statements. This makes
the write bound explicit at the formatting site rather than relying on
buffer sizing logic elsewhere in the function.
While the current buffer sizing rule (query_len + clocations_count * 10)
appears to account for worst-case growth, using snprintf provides local
verification of safety and protects against potential future modifications
to the sizing logic.
Additionally, improve NULL pointer safety for qbuffer:
- Initialize qbuffer to NULL in gc_qtexts()
- Set qbuffer to NULL after pfree() in both gc_qtexts() and
pgss_shmem_shutdown() error paths
These changes follow defensive programming practices and guard against
potential use-after-free scenarios.
This is defensive hardening only - no demonstrated overflow vulnerability
is being fixed. No performance impact expected.
---
contrib/pg_stat_statements/pg_stat_statements.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9231562..8ea847d 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -820,7 +820,10 @@ error:
errmsg("could not write file \"%s\": %m",
PGSS_DUMP_FILE ".tmp")));
if (qbuffer)
+ {
pfree(qbuffer);
+ qbuffer = NULL;
+ }
if (file)
FreeFile(file);
unlink(PGSS_DUMP_FILE ".tmp");
@@ -2475,7 +2478,7 @@ need_gc_qtexts(void)
static void
gc_qtexts(void)
{
- char *qbuffer;
+ char *qbuffer = NULL;
Size qbuffer_size;
FILE *qfile = NULL;
HASH_SEQ_STATUS hash_seq;
@@ -2590,6 +2593,7 @@ gc_qtexts(void)
pgss->mean_query_len = ASSUMED_LENGTH_INIT;
pfree(qbuffer);
+ qbuffer = NULL;
/*
* OK, count a garbage collection cycle. (Note: even though we have
@@ -2607,7 +2611,10 @@ gc_fail:
if (qfile)
FreeFile(qfile);
if (qbuffer)
+ {
pfree(qbuffer);
+ qbuffer = NULL;
+ }
/*
* Since the contents of the external file are now uncertain, mark all
@@ -2880,9 +2887,11 @@ generate_normalized_query(const JumbleState *jstate, const char *query,
* we have a squashable list, insert a placeholder comment starting
* from the list's second value.
*/
- n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d%s",
- num_constants_replaced + 1 + jstate->highest_extern_param_id,
- locs[i].squashed ? " /*, ... */" : "");
+ n_quer_loc += snprintf(norm_query + n_quer_loc,
+ norm_query_buflen - n_quer_loc + 1,
+ "$%d%s",
+ num_constants_replaced + 1 + jstate->highest_extern_param_id,
+ locs[i].squashed ? " /*, ... */" : "");
num_constants_replaced++;
/* move forward */
--
2.39.5 (Apple Git-154)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH] Defensive hardening: Replace sprintf with snprintf in pg_stat_statements
In-Reply-To: <CAEFUcvonaJe1fNZfGCEq3U8CKWFjC=W1JYSfUD7m_2Ox4_Cv-g@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox