> > > AFAICS, postgres_fdw's subsequent uses of those strings only need > > them to be nul-terminated C strings, so strncpy's property of > > zero-filling the whole buffer is not needed here. I recommend > > s/strncpy/strlcpy/. > > Seems like a good idea. > > > It's probably also appropriate to think about using pg_mbcliplen() > > to ensure that this code doesn't result in a broken multibyte > > character. > > Will do. > > I am currently overseas and will be attending an event this week, so I > will work on this after returning home. In the meantime, I created an > entry for it in the open items list. > > Thanks!
I've attached 2 different fixes. The first one swtiches to pg_mbcliplen+memcpy, which is probably overkill but better safe than sorry. The second gets rid of the string buffers entirely, and instead frees the nested object. I have no preference between the two, though I suspect that the nested-free solution will be preferred by the community.
From c1ec2ad6939b85dce3c692a1633e4d45facd292d Mon Sep 17 00:00:00 2001 From: Corey Huinker <[email protected]> Date: Thu, 30 Apr 2026 13:01:15 -0400 Subject: [PATCH v1] Replace strncpy with pg_mbcliplen plus memcpy. --- contrib/postgres_fdw/postgres_fdw.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index c42cb690c7b..ed72e02ced7 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -27,6 +27,7 @@ #include "executor/spi.h" #include "foreign/fdwapi.h" #include "funcapi.h" +#include "mb/pg_wchar.h" #include "miscadmin.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" @@ -5907,6 +5908,20 @@ fetch_attstats(PGconn *conn, int server_version_num, return res; } +/* + * Copy a untrusted string into a NameData conforming string buffer. + */ +static void +attname_copy(char *dest, char *src) +{ + int len = strlen(src); + + if (len >= NAMEDATALEN) + len = pg_mbcliplen(src, len, NAMEDATALEN - 1); + + memcpy(dest, src, len+1); +} + /* * Build the mapping of local columns to remote columns and create a column * list used for constructing the fetch_attstats query. @@ -5957,8 +5972,8 @@ build_remattrmap(Relation relation, List *va_cols, appendStringInfoString(column_list, quote_identifier(remote_attname)); remattrmap[attrcnt].local_attnum = attnum; - strncpy(remattrmap[attrcnt].local_attname, attname, NAMEDATALEN); - strncpy(remattrmap[attrcnt].remote_attname, remote_attname, NAMEDATALEN); + attname_copy(remattrmap[attrcnt].local_attname, attname); + attname_copy(remattrmap[attrcnt].remote_attname, remote_attname); remattrmap[attrcnt].res_index = -1; attrcnt++; } base-commit: 5642a0367c2fe69684800c5d5c5929c20d99ef72 -- 2.54.0
From 867d5b7dfbd73caa4d8e917c900e92159b4653ce Mon Sep 17 00:00:00 2001 From: Corey Huinker <[email protected]> Date: Thu, 30 Apr 2026 13:52:27 -0400 Subject: [PATCH v1] Replace name buffers from RemoteAttributeMapping with pointers Replace the char buffers in RemoteAttributeMapping with regular pointers. This creates the need for a function to clean up the nested data structure, but it elminates the need for some buffer checks that would have required including pg_wchar.h. --- contrib/postgres_fdw/postgres_fdw.c | 34 +++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index c42cb690c7b..8164e9b87f8 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -324,10 +324,10 @@ typedef struct /* Pairs of remote columns with local columns */ typedef struct { - AttrNumber local_attnum; - char local_attname[NAMEDATALEN]; - char remote_attname[NAMEDATALEN]; + char *local_attname; + char *remote_attname; int res_index; + AttrNumber local_attnum; } RemoteAttributeMapping; /* Result sets that are returned from a foreign statistics scan */ @@ -704,6 +704,7 @@ static PGresult *fetch_attstats(PGconn *conn, int server_version_num, const char *column_list); static RemoteAttributeMapping *build_remattrmap(Relation relation, List *va_cols, int *p_attrcnt, StringInfo column_list); +static void free_remattrmap(RemoteAttributeMapping *map, int len); static bool attname_in_list(const char *attname, List *va_cols); static int remattrmap_cmp(const void *v1, const void *v2); static bool match_attrmap(PGresult *res, @@ -5670,8 +5671,7 @@ postgresImportForeignStatistics(Relation relation, List *va_cols, int elevel) PQclear(remstats.rel); PQclear(remstats.att); - if (remattrmap) - pfree(remattrmap); + free_remattrmap(remattrmap, attrcnt); return ok; } @@ -5957,8 +5957,8 @@ build_remattrmap(Relation relation, List *va_cols, appendStringInfoString(column_list, quote_identifier(remote_attname)); remattrmap[attrcnt].local_attnum = attnum; - strncpy(remattrmap[attrcnt].local_attname, attname, NAMEDATALEN); - strncpy(remattrmap[attrcnt].remote_attname, remote_attname, NAMEDATALEN); + remattrmap[attrcnt].local_attname = pstrdup(attname); + remattrmap[attrcnt].remote_attname = pstrdup(remote_attname); remattrmap[attrcnt].res_index = -1; attrcnt++; } @@ -5972,6 +5972,26 @@ build_remattrmap(Relation relation, List *va_cols, return remattrmap; } +/* + * Free the structure created by build_remattrmap(). + */ +static void +free_remattrmap(RemoteAttributeMapping *map, int len) +{ + if (!map) + return; + + for (int i = 0; i < len; i++) + { + if (map[i].local_attname) + pfree(map[i].local_attname); + if (map[i].remote_attname) + pfree(map[i].remote_attname); + } + + pfree(map); +} + /* * Test if an attribute name is in the list. * base-commit: 5642a0367c2fe69684800c5d5c5929c20d99ef72 -- 2.54.0
