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 <[email protected]> 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
From de3ed61590ef80e2d28649aab722dc549b6576ea Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
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
