On Wed, 09 Apr 2025 at 10:34, Junwang Zhao <zhjw...@gmail.com> wrote: > On Wed, Apr 9, 2025 at 5:22 AM David Rowley <dgrowle...@gmail.com> wrote: >> >> On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <pe...@eisentraut.org> wrote: >> > To avoid creating an array on the stack, you could maybe write it with a >> > pointer instead, like: >> > >> > const char * const query = "..."; >> > >> > I haven't tested whether that results in different or better compiled >> > code. The original code is probably good enough. >> >> I expect it's been done the way it has to make the overflow detection >> code work. The problem with having a pointer to a string constant is >> that sizeof() will return the size of the pointer and not the space >> needed to store the string. >> >> We can get rid of the variable and make the overflow work by checking >> the return length of snprintf. I think that's all C99 spec now... >> >> len = snprintf(qbuf, "set client_encoding to '%s'", encoding); >> /* check query buffer overflow */ >> if (len >= sizeof(qbuf)) >> return -1; >> > > Agree, this is a better coding style. Please see if the following code makes > sense to you. > > diff --git a/src/interfaces/libpq/fe-connect.c > b/src/interfaces/libpq/fe-connect.c > index 0258d9ace3c..247d079faa6 100644 > --- a/src/interfaces/libpq/fe-connect.c > +++ b/src/interfaces/libpq/fe-connect.c > @@ -7717,9 +7717,9 @@ int > PQsetClientEncoding(PGconn *conn, const char *encoding) > { > char qbuf[128]; > - static const char query[] = "set client_encoding to '%s'"; > PGresult *res; > int status; > + int len; > > if (!conn || conn->status != CONNECTION_OK) > return -1; > @@ -7731,12 +7731,11 @@ PQsetClientEncoding(PGconn *conn, const char > *encoding) > if (strcmp(encoding, "auto") == 0) > encoding = > pg_encoding_to_char(pg_get_encoding_from_locale(NULL, true)); > > - /* check query buffer overflow */ > - if (sizeof(qbuf) < (sizeof(query) + strlen(encoding))) > + len = snprintf(qbuf, sizeof(qbuf), "set client_encoding to > '%s'", encoding); > + if (len >= sizeof(qbuf)) > return -1; > > /* ok, now send a query */ > - sprintf(qbuf, query, encoding); > res = PQexec(conn, qbuf); > > if (res == NULL) >
Thank you for all the explanations! The above code looks good to me, except for a minor issue; since snprintf may return a negative value, should we check for this? -- Regrads, Japin Li