Обсуждение: [PATCH] Fix incorrect fprintf usage in log_error FRONTEND path

Bryan Green <dbryan.green@gmail.com> writes:
> I noticed that the log_error() utility function has a bug in its FRONTEND
> code path. The function uses fprintf() with a va_list argument, but
> fprintf() expects individual arguments. This should
> be vfprintf() instead.

Huh, that certainly appears broken, but isn't the non-FRONTEND
case equally broken?  write_stderr() doesn't expect a va_list
either.  I wonder if this code is ever reached at all.

            regards, tom lane

On 10/13/25 13:16, Bryan Green wrote:
> On 10/13/25 13:01, Tom Lane wrote:
>> Bryan Green <dbryan.green@gmail.com> writes:
>>> On 10/13/25 10:48, Tom Lane wrote:
>>>> Huh, that certainly appears broken, but isn't the non-FRONTEND
>>>> case equally broken?  write_stderr() doesn't expect a va_list
>>>> either.  I wonder if this code is ever reached at all.
>>
>>> I found two instances of write_stderr(), one in elog.c and another in
>>> pg_ctl.c.
>>> They both use functions that correctly handle the va_list-- either
>>> vsnprintf or vfprintf.
>>
>> It's the one in elog.c that the non-FRONTEND case is going to invoke.
>> But I think you're mistaken that it'll "just work".  write_stderr()
>> is creating its own va_list.
>>
>> We probably need to invent vwrite_stderr().
>>
>>             regards, tom lane
> Oh, yes, I see it now.  I will create the vwrite_stderr()-- probably 
> something like below, and then create a new patch.
> 
> void
> vwrite_stderr(const char *fmt, va_list ap)
> {
> #ifdef WIN32
>      char        errbuf[2048];
> #endif
>      fmt = _(fmt);
> 
> #ifndef WIN32
>      vfprintf(stderr, fmt, ap);
>      fflush(stderr);
> #else
>      vsnprintf(errbuf, sizeof(errbuf), fmt, ap);
>      if (pgwin32_is_service())
>          write_eventlog(ERROR, errbuf, strlen(errbuf));
>      else
>      {
>          write_console(errbuf, strlen(errbuf));
>          fflush(stderr);
>      }
> #endif
> }
> 
> Thanks,
> Bryan Green
I mistakenly just hit reply instead of reply all.  Apologies for the 
clutter.

On 10/13/25 13:24, Bryan Green wrote:
> On 10/13/25 13:16, Bryan Green wrote:
>> On 10/13/25 13:01, Tom Lane wrote:
>>> Bryan Green <dbryan.green@gmail.com> writes:
>>>> On 10/13/25 10:48, Tom Lane wrote:
>>>>> Huh, that certainly appears broken, but isn't the non-FRONTEND
>>>>> case equally broken?  write_stderr() doesn't expect a va_list
>>>>> either.  I wonder if this code is ever reached at all.
>>>
>>>> I found two instances of write_stderr(), one in elog.c and another in
>>>> pg_ctl.c.
>>>> They both use functions that correctly handle the va_list-- either
>>>> vsnprintf or vfprintf.
>>>
>>> It's the one in elog.c that the non-FRONTEND case is going to invoke.
>>> But I think you're mistaken that it'll "just work".  write_stderr()
>>> is creating its own va_list.
>>>
>>> We probably need to invent vwrite_stderr().
>>>
>>>             regards, tom lane
>> Oh, yes, I see it now.  I will create the vwrite_stderr()-- probably 
>> something like below, and then create a new patch.
>>
>> void
>> vwrite_stderr(const char *fmt, va_list ap)
>> {
>> #ifdef WIN32
>>      char        errbuf[2048];
>> #endif
>>      fmt = _(fmt);
>>
>> #ifndef WIN32
>>      vfprintf(stderr, fmt, ap);
>>      fflush(stderr);
>> #else
>>      vsnprintf(errbuf, sizeof(errbuf), fmt, ap);
>>      if (pgwin32_is_service())
>>          write_eventlog(ERROR, errbuf, strlen(errbuf));
>>      else
>>      {
>>          write_console(errbuf, strlen(errbuf));
>>          fflush(stderr);
>>      }
>> #endif
>> }
>>
>> Thanks,
>> Bryan Green
> I mistakenly just hit reply instead of reply all.  Apologies for the 
> clutter.
Hello,

The problem:
- FRONTEND path: fprintf(stderr, fmt, ap) - incorrect
- non-FRONTEND path: write_stderr(fmt, ap) - also incorrect

The first patch only addressed the fprintf issue.  The v2 patch 
addresses both.

Both were passing a va_list to variadic functions that expect individual 
arguments (...), causing the va_list pointer to be treated as a single 
argument rather than properly expanding the arguments.

In order to straighten this out the following items were done:

1. Adding a new vwrite_stderr() function that accepts va_list, following
    the standard C library pattern (printf/vprintf, fprintf/vfprintf, etc.)

2. Refactoring write_stderr() to call vwrite_stderr() internally

3. Fixing both paths in log_error():
    - FRONTEND: fprintf -> vfprintf
    - non-FRONTEND: write_stderr -> vwrite_stderr

This bug likely went unnoticed because these error paths are only hit 
when Windows security API calls fail catastrophically (out of 
memory,corrupted security subsystem, etc.), which is extremely rare in 
practice.

Please review the attached v2 patch.

Thanks,
bg

Bryan Green <dbryan.green@gmail.com> writes:
> The problem:
> - FRONTEND path: fprintf(stderr, fmt, ap) - incorrect
> - non-FRONTEND path: write_stderr(fmt, ap) - also incorrect

> The first patch only addressed the fprintf issue.  The v2 patch 
> addresses both.

Actually, you'd managed to drop the fprintf->vfprintf change :-(.
I fixed that, pgindent'd it, wordsmithed the commit message,
and pushed.  Many thanks for finding this!

            regards, tom lane