Thread

[PATCH] Defensive hardening: Replace sprintf with snprintf in pg_stat_statements

anupam MEDIRATTA <mediratta01.pally@gmail.com> — 2026-05-25T10:29:28Z

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