On Wed, 09 Apr 2025 at 10:34, Junwang Zhao <zhjwpku@gmail.com> wrote:
> On Wed, Apr 9, 2025 at 5:22 AM David Rowley <dgrowleyml@gmail.com> wrote:
>>
>> On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <peter@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
