From 014d8c4d090e9267deaaa8460c7d9c615c655d5f Mon Sep 17 00:00:00 2001 From: David Rowley Date: Thu, 15 Sep 2022 21:27:00 +1200 Subject: [PATCH v1 1/3] Adjust code to highlight appendStringInfo misusages Not intended for commit --- src/backend/nodes/outfuncs.c | 40 ++++++++++++------------- src/backend/utils/adt/xml.c | 2 +- src/bin/psql/create_help.pl | 7 ++++- src/common/stringinfo.c | 6 ++-- src/fe_utils/string_utils.c | 4 +-- src/include/lib/stringinfo.h | 28 +++++++++++++++-- src/interfaces/libpq-oauth/oauth-curl.c | 6 ++-- src/interfaces/libpq/exports.txt | 4 +-- src/interfaces/libpq/pqexpbuffer.c | 6 ++-- src/interfaces/libpq/pqexpbuffer.h | 26 ++++++++++++++-- 10 files changed, 89 insertions(+), 40 deletions(-) diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 953c5797c5d..4b1e7675fc9 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -45,11 +45,11 @@ static void outDouble(StringInfo str, double d); /* Write an integer field (anything written as ":fldname %d") */ #define WRITE_INT_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %d", node->fldname) + appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", node->fldname) /* Write an unsigned integer field (anything written as ":fldname %u") */ #define WRITE_UINT_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname) + appendStringInfoInternal(str, " :" CppAsString(fldname) " %u", node->fldname) /* Write a signed integer field (anything written with INT64_FORMAT) */ #define WRITE_INT64_FIELD(fldname) \ @@ -59,84 +59,84 @@ static void outDouble(StringInfo str, double d); /* Write an unsigned integer field (anything written with UINT64_FORMAT) */ #define WRITE_UINT64_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \ + appendStringInfoInternal(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \ node->fldname) /* Write an OID field (don't hard-wire assumption that OID is same as uint) */ #define WRITE_OID_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname) + appendStringInfoInternal(str, " :" CppAsString(fldname) " %u", node->fldname) /* Write a long-integer field */ #define WRITE_LONG_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %ld", node->fldname) + appendStringInfoInternal(str, " :" CppAsString(fldname) " %ld", node->fldname) /* Write a char field (ie, one ascii character) */ #define WRITE_CHAR_FIELD(fldname) \ - (appendStringInfo(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ outChar(str, node->fldname)) /* Write an enumerated-type field as an integer code */ #define WRITE_ENUM_FIELD(fldname, enumtype) \ - appendStringInfo(str, " :" CppAsString(fldname) " %d", \ + appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", \ (int) node->fldname) /* Write a float field (actually, they're double) */ #define WRITE_FLOAT_FIELD(fldname) \ - (appendStringInfo(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoInternal(str, " :" CppAsString(fldname) " "), \ outDouble(str, node->fldname)) /* Write a boolean field */ #define WRITE_BOOL_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %s", \ + appendStringInfoInternal(str, " :" CppAsString(fldname) " %s", \ booltostr(node->fldname)) /* Write a character-string (possibly NULL) field */ #define WRITE_STRING_FIELD(fldname) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ outToken(str, node->fldname)) /* Write a parse location field (actually same as INT case) */ #define WRITE_LOCATION_FIELD(fldname) \ - appendStringInfo(str, " :" CppAsString(fldname) " %d", write_location_fields ? node->fldname : -1) + appendStringInfoInternal(str, " :" CppAsString(fldname) " %d", write_location_fields ? node->fldname : -1) /* Write a Node field */ #define WRITE_NODE_FIELD(fldname) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ outNode(str, node->fldname)) /* Write a bitmapset field */ #define WRITE_BITMAPSET_FIELD(fldname) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ outBitmapset(str, node->fldname)) /* Write a variable-length array (not a List) of Node pointers */ #define WRITE_NODE_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeNodeArray(str, (const Node * const *) node->fldname, len)) /* Write a variable-length array of AttrNumber */ #define WRITE_ATTRNUMBER_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeAttrNumberCols(str, node->fldname, len)) /* Write a variable-length array of Oid */ #define WRITE_OID_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeOidCols(str, node->fldname, len)) /* Write a variable-length array of Index */ #define WRITE_INDEX_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeIndexCols(str, node->fldname, len)) /* Write a variable-length array of int */ #define WRITE_INT_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeIntCols(str, node->fldname, len)) /* Write a variable-length array of bool */ #define WRITE_BOOL_ARRAY(fldname, len) \ - (appendStringInfoString(str, " :" CppAsString(fldname) " "), \ + (appendStringInfoStringInternal(str, " :" CppAsString(fldname) " "), \ writeBoolCols(str, node->fldname, len)) #define booltostr(x) ((x) ? "true" : "false") @@ -242,7 +242,7 @@ fnname(StringInfo str, const datatype *arr, int len) \ appendStringInfoChar(str, ')'); \ } \ else \ - appendStringInfoString(str, "<>"); \ + appendStringInfoStringInternal(str, "<>"); \ } WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",) diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c index 2c7f778cfdb..e4abb530bf6 100644 --- a/src/backend/utils/adt/xml.c +++ b/src/backend/utils/adt/xml.c @@ -2243,7 +2243,7 @@ xml_errorHandler(void *data, PgXmlErrorPtr error) void *errCtxSaved = xmlGenericErrorContext; xmlSetGenericErrorFunc(&errorBuf, - (xmlGenericErrorFunc) appendStringInfo); + (xmlGenericErrorFunc) appendStringInfoInternal); /* Add context information to errorBuf */ appendStringInfoLineSeparator(&errorBuf); diff --git a/src/bin/psql/create_help.pl b/src/bin/psql/create_help.pl index 12931c17b69..391aa97e2de 100644 --- a/src/bin/psql/create_help.pl +++ b/src/bin/psql/create_help.pl @@ -188,10 +188,15 @@ foreach (sort keys %entries) $synopsis =~ s/\\n/\\n"\n$prefix"/g; my @args = ("buf", $synopsis, map("_(\"$_\")", @{ $entries{$_}{params} })); + my $funcname = "appendPQExpBufferStr"; + if (scalar(@args) > 2) + { + $funcname = "appendPQExpBuffer"; + } print $cfile_handle "static void sql_help_$id(PQExpBuffer buf) { -\tappendPQExpBuffer(" . join(",\n$prefix", @args) . "); +\t$funcname(" . join(",\n$prefix", @args) . "); } "; diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c index ae39540e468..7a765165562 100644 --- a/src/common/stringinfo.c +++ b/src/common/stringinfo.c @@ -142,7 +142,7 @@ resetStringInfo(StringInfo str) * strcat. */ void -appendStringInfo(StringInfo str, const char *fmt,...) +appendStringInfoInternal(StringInfo str, const char *fmt, ...) { int save_errno = errno; @@ -221,13 +221,13 @@ appendStringInfoVA(StringInfo str, const char *fmt, va_list args) } /* - * appendStringInfoString + * appendStringInfoStringInternal * * Append a null-terminated string to str. * Like appendStringInfo(str, "%s", s) but faster. */ void -appendStringInfoString(StringInfo str, const char *s) +appendStringInfoStringInternal(StringInfo str, const char *s) { appendBinaryStringInfo(str, s, strlen(s)); } diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c index 38fffbd036b..5688ffd408c 100644 --- a/src/fe_utils/string_utils.c +++ b/src/fe_utils/string_utils.c @@ -1056,7 +1056,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern, int dcnt; #define WHEREAND() \ - (appendPQExpBufferStr(buf, have_where ? " AND " : "WHERE "), \ + (appendPQExpBufferStrInternal(buf, have_where ? " AND " : "WHERE "), \ have_where = true, added_clause = true) if (dotcnt == NULL) @@ -1068,7 +1068,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern, if (visibilityrule) { WHEREAND(); - appendPQExpBuffer(buf, "%s\n", visibilityrule); + appendPQExpBufferInternal(buf, "%s\n", visibilityrule); } return added_clause; } diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h index 079652c8ce4..7e5df622727 100644 --- a/src/include/lib/stringinfo.h +++ b/src/include/lib/stringinfo.h @@ -196,9 +196,20 @@ extern void resetStringInfo(StringInfo str); * to str if necessary. This is sort of like a combination of sprintf and * strcat. */ -extern void appendStringInfo(StringInfo str, const char *fmt,...) pg_attribute_printf(2, 3); +#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING) +#define appendStringInfo(str, fmt, ...) \ + do { \ + StaticAssertStmt(!__builtin_constant_p(fmt) || strcmp(fmt, "%s") != 0, \ + "use appendStringInfoString instead of appendStringInfo with %s"); \ + appendStringInfoInternal(str, fmt, __VA_ARGS__); \ + } while (0) +#else +#define appendStringInfo(str, fmt, ...) appendStringInfoInternal(str, fmt, __VA_ARGS__) +#endif -/*------------------------ +extern void appendStringInfoInternal(StringInfo str, const char *fmt,...) pg_attribute_printf(2, 3); + + /*------------------------ * appendStringInfoVA * Attempt to format text data under the control of fmt (an sprintf-style * format string) and append it to whatever is already in str. If successful @@ -214,7 +225,18 @@ extern int appendStringInfoVA(StringInfo str, const char *fmt, va_list args) pg_ * Append a null-terminated string to str. * Like appendStringInfo(str, "%s", s) but faster. */ -extern void appendStringInfoString(StringInfo str, const char *s); +#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING) +#define appendStringInfoString(str, s) \ + do { \ + StaticAssertStmt(!__builtin_constant_p(s) || strlen(s) != 1, \ + "use appendStringInfoChar to append single characters to a string"); \ + appendStringInfoStringInternal(str, s); \ + } while (0) +#else +#define appendStringInfoString(str, s) appendStringInfoStringInternal(str, s) +#endif + +extern void appendStringInfoStringInternal(StringInfo str, const char *s); /*------------------------ * appendStringInfoChar diff --git a/src/interfaces/libpq-oauth/oauth-curl.c b/src/interfaces/libpq-oauth/oauth-curl.c index abbef93f95f..ea9518a4288 100644 --- a/src/interfaces/libpq-oauth/oauth-curl.c +++ b/src/interfaces/libpq-oauth/oauth-curl.c @@ -417,13 +417,13 @@ append_actx_error(PGoauthBearerRequestV2 *req, struct async_ctx *actx) */ #define actx_error(ACTX, FMT, ...) \ - appendPQExpBuffer(&(ACTX)->errbuf, libpq_gettext(FMT), ##__VA_ARGS__) + appendPQExpBufferInternal(&(ACTX)->errbuf, libpq_gettext(FMT), ##__VA_ARGS__) #define actx_error_internal(ACTX, FMT, ...) \ appendPQExpBuffer(&(ACTX)->errbuf, FMT, ##__VA_ARGS__) #define actx_error_str(ACTX, S) \ - appendPQExpBufferStr(&(ACTX)->errbuf, S) + appendPQExpBufferStrInternal(&(ACTX)->errbuf, S) /* * Macros for getting and setting state for the connection's two libcurl @@ -507,7 +507,7 @@ struct oauth_parse }; #define oauth_parse_set_error(ctx, fmt, ...) \ - appendPQExpBuffer((ctx)->errbuf, libpq_gettext(fmt), ##__VA_ARGS__) + appendPQExpBufferInternal((ctx)->errbuf, libpq_gettext(fmt), ##__VA_ARGS__) #define oauth_parse_set_error_internal(ctx, fmt, ...) \ appendPQExpBuffer((ctx)->errbuf, fmt, ##__VA_ARGS__) diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index 1e3d5bd5867..0bcfe857914 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -74,7 +74,7 @@ PQoidValue 71 PQclientEncoding 72 PQenv2encoding 73 appendBinaryPQExpBuffer 74 -appendPQExpBufferStr 75 +appendPQExpBufferStrInternal 75 destroyPQExpBuffer 76 createPQExpBuffer 77 PQconninfoFree 78 @@ -90,7 +90,7 @@ PQfreeNotify 87 PQescapeString 88 PQescapeBytea 89 printfPQExpBuffer 90 -appendPQExpBuffer 91 +appendPQExpBufferInternal 91 pg_encoding_to_char 92 pg_utf_mblen 93 PQunescapeBytea 94 diff --git a/src/interfaces/libpq/pqexpbuffer.c b/src/interfaces/libpq/pqexpbuffer.c index 153ae6af6cb..a7730118127 100644 --- a/src/interfaces/libpq/pqexpbuffer.c +++ b/src/interfaces/libpq/pqexpbuffer.c @@ -254,7 +254,7 @@ printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) } /* - * appendPQExpBuffer + * appendPQExpBufferInternal * * Format text data under the control of fmt (an sprintf-like format string) * and append it to whatever is already in str. More space is allocated @@ -262,7 +262,7 @@ printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) * strcat. */ void -appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) +appendPQExpBufferInternal(PQExpBuffer str, const char *fmt,...) { int save_errno = errno; va_list args; @@ -364,7 +364,7 @@ appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) * if necessary. */ void -appendPQExpBufferStr(PQExpBuffer str, const char *data) +appendPQExpBufferStrInternal(PQExpBuffer str, const char *data) { appendBinaryPQExpBuffer(str, data, strlen(data)); } diff --git a/src/interfaces/libpq/pqexpbuffer.h b/src/interfaces/libpq/pqexpbuffer.h index dc83f4ffe86..57df4104b8f 100644 --- a/src/interfaces/libpq/pqexpbuffer.h +++ b/src/interfaces/libpq/pqexpbuffer.h @@ -155,7 +155,18 @@ extern void printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) pg_attribute * to str if necessary. This is sort of like a combination of sprintf and * strcat. */ -extern void appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) pg_attribute_printf(2, 3); +#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING) +#define appendPQExpBuffer(str, fmt, ...) \ + do { \ + StaticAssertStmt(!__builtin_constant_p(fmt) || strcmp(fmt, "%s") != 0, \ + "use appendPQExpBufferStr instead of appendPQExpBuffer with %s"); \ + appendPQExpBufferInternal(str, fmt, __VA_ARGS__); \ + } while (0) +#else +#define appendPQExpBuffer(str, fmt, ...) appendPQExpBufferInternal(str, fmt, __VA_ARGS__) +#endif + +extern void appendPQExpBufferInternal(PQExpBuffer str, const char *fmt,...) pg_attribute_printf(2, 3); /*------------------------ * appendPQExpBufferVA @@ -167,12 +178,23 @@ extern void appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) pg_attribute */ extern bool appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) pg_attribute_printf(2, 0); +#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(USE_ASSERT_CHECKING) +#define appendPQExpBufferStr(str, s) \ + do { \ + StaticAssertStmt(!__builtin_constant_p(s) || strlen(s) != 1, \ + "use appendPQExpBufferChar to append single characters to a string"); \ + appendPQExpBufferStrInternal(str, s); \ + } while (0) +#else +#define appendPQExpBufferStr(str, s) appendPQExpBufferStrInternal(str, s) +#endif + /*------------------------ * appendPQExpBufferStr * Append the given string to a PQExpBuffer, allocating more space * if necessary. */ -extern void appendPQExpBufferStr(PQExpBuffer str, const char *data); +extern void appendPQExpBufferStrInternal(PQExpBuffer str, const char *data); /*------------------------ * appendPQExpBufferChar -- 2.51.0