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


Reply via email to