[PATCH] Fix incorrect fprintf usage in log_error FRONTEND path
Hi,
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.
The relevant code:
#else
fprintf(stderr, fmt, ap);
#endif
Should be:
#else
vfprintf(stderr, fmt, ap);
#endif
While this code path may not currently be compiled in practice, it would
cause compilation warnings or runtime crashes if the FRONTEND macro is ever
defined for files containing this function.
I've attached a patch that fixes this issue.
Bryan Green
Attachments:
0001-Fix-incorrect-fprintf-usage-in-log_error-FRONTEND-pa.patchapplication/octet-stream; name=0001-Fix-incorrect-fprintf-usage-in-log_error-FRONTEND-pa.patchDownload
From 4c9f506b2b4ef4edbb1432f07e82da3ece0ed17b Mon Sep 17 00:00:00 2001
From: Bryan Green <dbryan.green@gmail.com>
Date: Mon, 13 Oct 2025 09:56:06 -0500
Subject: [PATCH] Fix incorrect fprintf usage in log_error FRONTEND path
The FRONTEND branch of log_error() was calling fprintf() with a
va_list argument, but fprintf() expects individual arguments.
This should use vfprintf() instead to properly handle the va_list.
While this code path may not be currently compiled in practice,
it would fail to compile or crash at runtime if FRONTEND were
ever defined for files containing this function.
---
src/port/win32security.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/port/win32security.c b/src/port/win32security.c
index a46b82dd04..c531a4fc05 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -33,7 +33,7 @@ log_error(const char *fmt,...)
#ifndef FRONTEND
write_stderr(fmt, ap);
#else
- fprintf(stderr, fmt, ap);
+ vfprintf(stderr, fmt, ap);
#endif
va_end(ap);
}
--
2.49.0
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.
Import Notes
Reply to msg id not found: 4fc6f5d3-e487-46cf-93e4-4896e980aa94@gmail.com
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 GreenI 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
Attachments:
v2-0001-Fix-incorrect-fprintf-usage-in-log_error-FRONTEND-pa.patchtext/plain; charset=UTF-8; name=v2-0001-Fix-incorrect-fprintf-usage-in-log_error-FRONTEND-pa.patchDownload
From de3ed61590ef80e2d28649aab722dc549b6576ea Mon Sep 17 00:00:00 2001
From: Bryan Green <dbryan.green@gmail.com>
Date: Mon, 13 Oct 2025 14:53:58 -0500
Subject: [PATCH] Fix va_list handling in log_error() and add vwrite_stderr()
The log_error() function in win32security.c was incorrectly passing
a va_list to variadic functions, which would cause the va_list pointer
to be treated as a single argument rather than expanding the arguments
properly.
Specifically:
- In the non-FRONTEND path, it called write_stderr(fmt, ap) where
write_stderr() expects variadic arguments (...), not a va_list
- In the FRONTEND path, it called fprintf(stderr, fmt, ap) where
fprintf() also expects variadic arguments, not a va_list
This bug likely went undetected because the error paths in
pgwin32_is_admin() are rarely exercised - they only trigger when
Windows security API calls fail catastrophically (out of memory,
security subsystem corruption, etc.).
To fix this, add a new vwrite_stderr() function that accepts a
va_list parameter, following the standard C library pattern of
providing both variadic and va_list versions (like printf/vprintf,
fprintf/vfprintf, etc.).
Changes:
- Add vwrite_stderr() to handle va_list arguments
- Refactor write_stderr() to call vwrite_stderr() internally
- Fix log_error() to use vwrite_stderr() for non-FRONTEND path
- Fix log_error() to use vfprintf() for FRONTEND path
This ensures proper argument expansion in both code paths.
---
src/backend/utils/error/elog.c | 16 +++++++++++++---
src/include/utils/elog.h | 1 +
src/port/win32security.c | 2 +-
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b7b9692f8c..399ab67fcf 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3786,13 +3786,24 @@ write_stderr(const char *fmt,...)
{
va_list ap;
+ va_start(ap, fmt);
+ vwrite_stderr(fmt, ap);
+ va_end(ap);
+}
+
+
+/*
+ * Write errors to stderr (or by equal means when stderr is
+ * not available) - va_list version
+ */
+void
+vwrite_stderr(const char *fmt,va_list ap)
+{
#ifdef WIN32
char errbuf[2048]; /* Arbitrary size? */
#endif
fmt = _(fmt);
-
- va_start(ap, fmt);
#ifndef WIN32
/* On Unix, we just fprintf to stderr */
vfprintf(stderr, fmt, ap);
@@ -3815,5 +3826,4 @@ write_stderr(const char *fmt,...)
fflush(stderr);
}
#endif
- va_end(ap);
}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 348dafbf90..001ab93ae6 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -528,5 +528,6 @@ extern void write_jsonlog(ErrorData *edata);
* safely (memory context, GUC load etc)
*/
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void vwrite_stderr(const char *fmt, va_list ap) pg_attribute_printf(1, 0);
#endif /* ELOG_H */
diff --git a/src/port/win32security.c b/src/port/win32security.c
index a46b82dd04..af3537ab82 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -31,7 +31,7 @@ log_error(const char *fmt,...)
va_start(ap, fmt);
#ifndef FRONTEND
- write_stderr(fmt, ap);
+ vwrite_stderr(fmt, ap);
#else
fprintf(stderr, fmt, ap);
#endif
--
2.49.0
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