> On Aug 4, 2026, at 17:36, Peter Eisentraut <[email protected]> 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/
nocfbot-0001-addition.diff
Description: Binary data
nocfbot-0003-addition.diff
Description: Binary data
