Make printTableAddCell/printTableAddHeader string argument const
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253306psql -h localhost -U postgresBuilt from patchset v3 (message #3), August 11, 2026 at 03:20 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253306_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253306_3 && git checkout t253306_3Patchset v3 (message #3) is on t253306_3
These functions are used in psql to assemble tables to print.
They would sometimes overwrite the string argument they are passed,
namely via mbvalidate(), which removes invalid UTF-8 characters (or
potentially analogously in other encodings, but that is not
implemented). However, many callers are not expecting that. In many
callers, the input value comes directly from libpq structures, such as
from PQgetvalue() or PQsslAttribute(). The latter actually has a const
char * return type, and that was cast away. But even the former is not
expecting its return value to be modified.
Fix that by making these arguments const. Internally, we add a separate
function that does only the checking part of mbvalidate(). Only if the
validation returns a negative result, we make a copy and run
mbvalidate() on the copy. printTableAddCell() already had internal
infrastructure for keeping track of what values needed to be freed. We
add the same for printTableAddHeader().
In passing, also simplify the code a bit. There were essentially
duplicate mechanisms for keeping track of the most recently added
cell (fields .cell and .cellsadded). Make that consistent by using an
integer counter for everything. That makes the code arguably easier to
read than with the "current pointer" approaches.
The first three patches are preparation patches to further clean up the
nearby code a bit.
Attachments:
t253306_10001-Remove-useless-confusing-const-qualifiers.patchtext/plain; charset=UTF-8; name=0001-Remove-useless-confusing-const-qualifiers.patchDownload+18-19
0002-Remove-useless-ENABLE_NLS-conditionals.patchtext/plain; charset=UTF-8; name=0002-Remove-useless-ENABLE_NLS-conditionals.patchDownload+1-13
0003-Use-frontend-logging-API-in-fe_utils-print.c.patchtext/plain; charset=UTF-8; name=0003-Use-frontend-logging-API-in-fe_utils-print.c.patchDownload+12-21
0004-Make-printTableAddCell-printTableAddHeader-string-ar.patchtext/plain; charset=UTF-8; name=0004-Make-printTableAddCell-printTableAddHeader-string-ar.patchDownload+116-38
On Aug 4, 2026, at 17:36, Peter Eisentraut <peter@eisentraut.org> wrote:
These functions are used in psql to assemble tables to print.
They would sometimes overwrite the string argument they are passed, namely via mbvalidate(), which removes invalid UTF-8 characters (or potentially analogously in other encodings, but that is not implemented). However, many callers are not expecting that. In many callers, the input value comes directly from libpq structures, such as from PQgetvalue() or PQsslAttribute(). The latter actually has a const char * return type, and that was cast away. But even the former is not expecting its return value to be modified.
Fix that by making these arguments const. Internally, we add a separate function that does only the checking part of mbvalidate(). Only if the validation returns a negative result, we make a copy and run mbvalidate() on the copy. printTableAddCell() already had internal infrastructure for keeping track of what values needed to be freed. We add the same for printTableAddHeader().
In passing, also simplify the code a bit. There were essentially duplicate mechanisms for keeping track of the most recently added
cell (fields .cell and .cellsadded). Make that consistent by using an integer counter for everything. That makes the code arguably easier to read than with the "current pointer" approaches.The first three patches are preparation patches to further clean up the nearby code a bit.
<0001-Remove-useless-confusing-const-qualifiers.patch><0002-Remove-useless-ENABLE_NLS-conditionals.patch><0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><0004-Make-printTableAddCell-printTableAddHeader-string-ar.patch>
0001, 0002 and 0003 look good to me.
For 0001 and 0003, I searched over the source tree, and found a few more occurrences, see the attached diff files.
For 0004, it seems to introduce a memory leak in printTableAddCell():
```
/*
@@ -3284,7 +3306,7 @@ printTableAddHeader(printTableContent *content, char *header,
* Note: Automatic freeing of translatable strings is not supported.
*/
void
-printTableAddCell(printTableContent *content, char *cell,
+printTableAddCell(printTableContent *content, const char *cell,
bool translate, bool mustfree)
{
uint64 total_cells;
@@ -3295,11 +3317,26 @@ printTableAddCell(printTableContent *content, char *cell,
pg_fatal("cannot add cell to table content: total cell count of %" PRIu64 " exceeded",
total_cells);- *content->cell = (char *) mbvalidate((unsigned char *) cell,
- content->opt->encoding);
+ Assert(!(translate && mustfree)); if (translate)
- *content->cell = _(*content->cell);
+ cell = _(cell);
+
+ /*
+ * Note: Translated strings are not checked for encoding validity. These
+ * are provided by ourselves, so they had better be ok. And if they were
+ * not, running mbvalidate on them could overwrite gettext-owned memory.
+ */
+ if (!translate && !mb_is_valid((unsigned char *) cell, content->opt->encoding))
+ {
+ char *cell2;
+
+ cell2 = pg_strdup(cell);
+ cell = (char *) mbvalidate((unsigned char *) cell2, content->opt->encoding);
+ mustfree = true;
+ }
+
+ content->cells[content->cellsadded] = cell; if (mustfree)
{
@@ -3309,7 +3346,7 @@ printTableAddCell(printTableContent *content, char *cell,
content->cellmustfree[content->cellsadded] = true;
}
- content->cell++;
+
content->cellsadded++;
}
```
When “translate" is false and “cell" contains invalid data, “cell2" is allocated and then replaces the original “cell". However, if “mustfree” was already true, the original “cell" should be freed. Since its pointer is overwritten, that memory is leaked.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 05.08.26 05:36, Chao Li wrote:
On Aug 4, 2026, at 17:36, Peter Eisentraut <peter@eisentraut.org> wrote:
These functions are used in psql to assemble tables to print.
They would sometimes overwrite the string argument they are passed, namely via mbvalidate(), which removes invalid UTF-8 characters (or potentially analogously in other encodings, but that is not implemented). However, many callers are not expecting that. In many callers, the input value comes directly from libpq structures, such as from PQgetvalue() or PQsslAttribute(). The latter actually has a const char * return type, and that was cast away. But even the former is not expecting its return value to be modified.
Fix that by making these arguments const. Internally, we add a separate function that does only the checking part of mbvalidate(). Only if the validation returns a negative result, we make a copy and run mbvalidate() on the copy. printTableAddCell() already had internal infrastructure for keeping track of what values needed to be freed. We add the same for printTableAddHeader().
In passing, also simplify the code a bit. There were essentially duplicate mechanisms for keeping track of the most recently added
cell (fields .cell and .cellsadded). Make that consistent by using an integer counter for everything. That makes the code arguably easier to read than with the "current pointer" approaches.The first three patches are preparation patches to further clean up the nearby code a bit.
<0001-Remove-useless-confusing-const-qualifiers.patch><0002-Remove-useless-ENABLE_NLS-conditionals.patch><0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><0004-Make-printTableAddCell-printTableAddHeader-string-ar.patch>0001, 0002 and 0003 look good to me.
For 0001 and 0003, I searched over the source tree, and found a few more occurrences, see the attached diff files.
Ok, I added these. (I added your 0001 to my 0001 and added your 0003 as
a separate patch.)
Note that your 0001 was incomplete: It did not update the "const bool
newline" in the add_tablespace_footer() definition.
Also, in your 0003, the messages still contained newlines, but the
logging API adds its own newlines and in fact rejects strings that end
with newlines, so that patch wouldn't have worked. I have fixed that.
For 0004, it seems to introduce a memory leak in printTableAddCell():
Thanks, I have fixed that in the attached patch.
Attachments:
t253306_3v2-0001-Remove-useless-confusing-const-qualifiers.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-useless-confusing-const-qualifiers.patchDownload+26-27
v2-0002-Remove-useless-ENABLE_NLS-conditionals.patchtext/plain; charset=UTF-8; name=v2-0002-Remove-useless-ENABLE_NLS-conditionals.patchDownload+1-13
v2-0003-Use-frontend-logging-API-in-fe_utils-print.c.patchtext/plain; charset=UTF-8; name=v2-0003-Use-frontend-logging-API-in-fe_utils-print.c.patchDownload+12-21
v2-0004-Make-printTableAddCell-printTableAddHeader-string.patchtext/plain; charset=UTF-8; name=v2-0004-Make-printTableAddCell-printTableAddHeader-string.patchDownload+125-38
v2-0005-Use-frontend-logging-API-in-fe_utils-string_utils.patchtext/plain; charset=UTF-8; name=v2-0005-Use-frontend-logging-API-in-fe_utils-string_utils.patchDownload+3-13
On Aug 6, 2026, at 03:44, Peter Eisentraut <peter@eisentraut.org> wrote:
On 05.08.26 05:36, Chao Li wrote:
On Aug 4, 2026, at 17:36, Peter Eisentraut <peter@eisentraut.org> wrote:
These functions are used in psql to assemble tables to print.
They would sometimes overwrite the string argument they are passed, namely via mbvalidate(), which removes invalid UTF-8 characters (or potentially analogously in other encodings, but that is not implemented). However, many callers are not expecting that. In many callers, the input value comes directly from libpq structures, such as from PQgetvalue() or PQsslAttribute(). The latter actually has a const char * return type, and that was cast away. But even the former is not expecting its return value to be modified.
Fix that by making these arguments const. Internally, we add a separate function that does only the checking part of mbvalidate(). Only if the validation returns a negative result, we make a copy and run mbvalidate() on the copy. printTableAddCell() already had internal infrastructure for keeping track of what values needed to be freed. We add the same for printTableAddHeader().
In passing, also simplify the code a bit. There were essentially duplicate mechanisms for keeping track of the most recently added
cell (fields .cell and .cellsadded). Make that consistent by using an integer counter for everything. That makes the code arguably easier to read than with the "current pointer" approaches.The first three patches are preparation patches to further clean up the nearby code a bit.
<0001-Remove-useless-confusing-const-qualifiers.patch><0002-Remove-useless-ENABLE_NLS-conditionals.patch><0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><0004-Make-printTableAddCell-printTableAddHeader-string-ar.patch>0001, 0002 and 0003 look good to me.
For 0001 and 0003, I searched over the source tree, and found a few more occurrences, see the attached diff files.Ok, I added these. (I added your 0001 to my 0001 and added your 0003 as a separate patch.)
Note that your 0001 was incomplete: It did not update the "const bool newline" in the add_tablespace_footer() definition.
Also, in your 0003, the messages still contained newlines, but the logging API adds its own newlines and in fact rejects strings that end with newlines, so that patch wouldn't have worked. I have fixed that.
Sorry about that. I was just about to point out what was missing, so I made those quick-and-dirty changes without reviewing them as carefully as I would when proposing a patch.
For 0004, it seems to introduce a memory leak in printTableAddCell():
Thanks, I have fixed that in the attached patch.
<v2-0001-Remove-useless-confusing-const-qualifiers.patch><v2-0002-Remove-useless-ENABLE_NLS-conditionals.patch><v2-0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><v2-0004-Make-printTableAddCell-printTableAddHeader-string.patch><v2-0005-Use-frontend-logging-API-in-fe_utils-string_utils.patch>
V2 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 06.08.26 02:29, Chao Li wrote:
On Aug 6, 2026, at 03:44, Peter Eisentraut <peter@eisentraut.org> wrote:
On 05.08.26 05:36, Chao Li wrote:
On Aug 4, 2026, at 17:36, Peter Eisentraut <peter@eisentraut.org> wrote:
These functions are used in psql to assemble tables to print.
They would sometimes overwrite the string argument they are passed, namely via mbvalidate(), which removes invalid UTF-8 characters (or potentially analogously in other encodings, but that is not implemented). However, many callers are not expecting that. In many callers, the input value comes directly from libpq structures, such as from PQgetvalue() or PQsslAttribute(). The latter actually has a const char * return type, and that was cast away. But even the former is not expecting its return value to be modified.
Fix that by making these arguments const. Internally, we add a separate function that does only the checking part of mbvalidate(). Only if the validation returns a negative result, we make a copy and run mbvalidate() on the copy. printTableAddCell() already had internal infrastructure for keeping track of what values needed to be freed. We add the same for printTableAddHeader().
In passing, also simplify the code a bit. There were essentially duplicate mechanisms for keeping track of the most recently added
cell (fields .cell and .cellsadded). Make that consistent by using an integer counter for everything. That makes the code arguably easier to read than with the "current pointer" approaches.The first three patches are preparation patches to further clean up the nearby code a bit.
<0001-Remove-useless-confusing-const-qualifiers.patch><0002-Remove-useless-ENABLE_NLS-conditionals.patch><0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><0004-Make-printTableAddCell-printTableAddHeader-string-ar.patch>0001, 0002 and 0003 look good to me.
For 0001 and 0003, I searched over the source tree, and found a few more occurrences, see the attached diff files.Ok, I added these. (I added your 0001 to my 0001 and added your 0003 as a separate patch.)
Note that your 0001 was incomplete: It did not update the "const bool newline" in the add_tablespace_footer() definition.
Also, in your 0003, the messages still contained newlines, but the logging API adds its own newlines and in fact rejects strings that end with newlines, so that patch wouldn't have worked. I have fixed that.
Sorry about that. I was just about to point out what was missing, so I made those quick-and-dirty changes without reviewing them as carefully as I would when proposing a patch.
For 0004, it seems to introduce a memory leak in printTableAddCell():
Thanks, I have fixed that in the attached patch.
<v2-0001-Remove-useless-confusing-const-qualifiers.patch><v2-0002-Remove-useless-ENABLE_NLS-conditionals.patch><v2-0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><v2-0004-Make-printTableAddCell-printTableAddHeader-string.patch><v2-0005-Use-frontend-logging-API-in-fe_utils-string_utils.patch>V2 LGTM.
Committed, thanks.