Clean up some signal usage mainly related to Windows
Windows has support for some signals[0]https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal, like SIGTERM and SIGINT. SIGINT
must be handled with care on Windows since it is handled in a separate
thread. SIGTERM however can be handled in a similar way to UNIX-like
systems. I audited a few pqsignal calls that were blocked by WIN32 to
see if they could become used, and made some adjustments. Definitely
hoping for someone with more Windows knowledge to audit this.
In addition, I found that signal_cleanup() in pg_test_fsync.c was not
using signal-safe functions, so I went ahead and fixed those to use
their signal-safe equivalents.
[0]: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v1-0001-Use-signal-safe-functions-in-signal-handler.patchtext/x-patch; charset=utf-8; name=v1-0001-Use-signal-safe-functions-in-signal-handler.patchDownload
From 2c4573afe55e83d3b5c99460c7429ff7cf3adcdf Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:25:14 -0500
Subject: [PATCH v1 1/2] Use signal-safe functions in signal handler
According to signal-safety(7), exit(3) and puts(3) are not safe to call
in a signal handler.
---
src/bin/pg_test_fsync/pg_test_fsync.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 435df8d808..2fc4b69444 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -596,8 +596,8 @@ signal_cleanup(SIGNAL_ARGS)
if (needs_unlink)
unlink(filename);
/* Finish incomplete line on stdout */
- puts("");
- exit(1);
+ write(STDOUT_FILENO, "", 1);
+ _exit(1);
}
#ifdef HAVE_FSYNC_WRITETHROUGH
--
Tristan Partin
Neon (https://neon.tech)
v1-0002-Cleanup-some-signal-usage-on-Windows.patchtext/x-patch; charset=utf-8; name=v1-0002-Cleanup-some-signal-usage-on-Windows.patchDownload
From ce4cbf8e720301e7ddbd2f0816c1f03c82138515 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:17:36 -0500
Subject: [PATCH v1 2/2] Cleanup some signal usage on Windows
SIGTERM can be handled the same as on a UNIX-like system. SIGINT, on the
other hand, requires special considerations. The Windows documentation
says the following:
> SIGINT is not supported for any Win32 application. When a CTRL+C
> interrupt occurs, Win32 operating systems generate a new thread to
> specifically handle that interrupt. This can cause a single-thread
> application, such as one in UNIX, to become multithreaded and cause
> unexpected behavior.
Therefore, if all the SIGINT handler does is read from memory and/or
exit, we can use the same handler. If the signal handler writes to
memory, such as setting a boolean, the original thread would never
become aware of the changed state.
---
src/bin/initdb/initdb.c | 17 ++++-------------
src/bin/pg_basebackup/pg_receivewal.c | 5 +----
src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++-----
src/bin/pg_test_fsync/pg_test_fsync.c | 3 ---
4 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..cac58eae2a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2773,26 +2773,17 @@ void
setup_signals(void)
{
/* some of these are not valid on Windows */
-#ifdef SIGHUP
- pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
+ pqsignal(SIGTERM, trapsig);
+
+#ifndef WIN32
pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
+ pqsignal(SIGHUP, trapsig);
pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
- pqsignal(SIGTERM, trapsig);
-#endif
/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
-#endif
/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
pqsignal(SIGSYS, SIG_IGN);
#endif
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..55143c4037 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -611,14 +611,11 @@ StreamLog(void)
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
* possible moment.
*/
-#ifndef WIN32
-
static void
sigexit_handler(SIGNAL_ARGS)
{
time_to_stop = true;
}
-#endif
int
main(int argc, char **argv)
@@ -838,9 +835,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..1228dc03db 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -644,10 +644,6 @@ error:
conn = NULL;
}
-/*
- * Unfortunately we can't do sensible signal handling on windows...
- */
-#ifndef WIN32
/*
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
@@ -659,6 +655,9 @@ sigexit_handler(SIGNAL_ARGS)
time_to_abort = true;
}
+/* SIGHUP is not defined on Windows */
+#ifndef WIN32
+
/*
* Trigger the output file to be reopened.
*/
@@ -921,9 +920,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 2fc4b69444..263f5867a2 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -109,9 +109,6 @@ main(int argc, char *argv[])
pqsignal(SIGTERM, signal_cleanup);
#ifndef WIN32
pqsignal(SIGALRM, process_alarm);
-#endif
-#ifdef SIGHUP
- /* Not defined on win32 */
pqsignal(SIGHUP, signal_cleanup);
#endif
--
Tristan Partin
Neon (https://neon.tech)
On 06.07.23 22:43, Tristan Partin wrote:
/* Finish incomplete line on stdout */ - puts(""); - exit(1); + write(STDOUT_FILENO, "", 1); + _exit(1);
puts() writes a newline, so it should probably be something like
write(STDOUT_FILENO, "\n", 1);
On Wed Jul 12, 2023 at 3:56 AM CDT, Peter Eisentraut wrote:
On 06.07.23 22:43, Tristan Partin wrote:
/* Finish incomplete line on stdout */ - puts(""); - exit(1); + write(STDOUT_FILENO, "", 1); + _exit(1);puts() writes a newline, so it should probably be something like
write(STDOUT_FILENO, "\n", 1);
Silly mistake. Thanks. v2 attached.
It has come to my attention that STDOUT_FILENO might not be portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw 1 for
stdout, which as far as I know is portable.
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v2-0002-Cleanup-some-signal-usage-on-Windows.patchtext/x-patch; charset=utf-8; name=v2-0002-Cleanup-some-signal-usage-on-Windows.patchDownload
From 2ec5f78e8c8b54ea8953a943d672d02b0d6f448d Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:17:36 -0500
Subject: [PATCH v2 2/2] Cleanup some signal usage on Windows
SIGTERM can be handled the same as on a UNIX-like system. SIGINT, on the
other hand, requires special considerations. The Windows documentation
says the following:
> SIGINT is not supported for any Win32 application. When a CTRL+C
> interrupt occurs, Win32 operating systems generate a new thread to
> specifically handle that interrupt. This can cause a single-thread
> application, such as one in UNIX, to become multithreaded and cause
> unexpected behavior.
Therefore, if all the SIGINT handler does is read from memory and/or
exit, we can use the same handler. If the signal handler writes to
memory, such as setting a boolean, the original thread would never
become aware of the changed state.
---
src/bin/initdb/initdb.c | 17 ++++-------------
src/bin/pg_basebackup/pg_receivewal.c | 5 +----
src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++-----
src/bin/pg_test_fsync/pg_test_fsync.c | 3 ---
4 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..cac58eae2a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2773,26 +2773,17 @@ void
setup_signals(void)
{
/* some of these are not valid on Windows */
-#ifdef SIGHUP
- pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
+ pqsignal(SIGTERM, trapsig);
+
+#ifndef WIN32
pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
+ pqsignal(SIGHUP, trapsig);
pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
- pqsignal(SIGTERM, trapsig);
-#endif
/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
-#endif
/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
pqsignal(SIGSYS, SIG_IGN);
#endif
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..55143c4037 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -611,14 +611,11 @@ StreamLog(void)
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
* possible moment.
*/
-#ifndef WIN32
-
static void
sigexit_handler(SIGNAL_ARGS)
{
time_to_stop = true;
}
-#endif
int
main(int argc, char **argv)
@@ -838,9 +835,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..1228dc03db 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -644,10 +644,6 @@ error:
conn = NULL;
}
-/*
- * Unfortunately we can't do sensible signal handling on windows...
- */
-#ifndef WIN32
/*
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
@@ -659,6 +655,9 @@ sigexit_handler(SIGNAL_ARGS)
time_to_abort = true;
}
+/* SIGHUP is not defined on Windows */
+#ifndef WIN32
+
/*
* Trigger the output file to be reopened.
*/
@@ -921,9 +920,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 42d0845b06..a3787d6d39 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -109,9 +109,6 @@ main(int argc, char *argv[])
pqsignal(SIGTERM, signal_cleanup);
#ifndef WIN32
pqsignal(SIGALRM, process_alarm);
-#endif
-#ifdef SIGHUP
- /* Not defined on win32 */
pqsignal(SIGHUP, signal_cleanup);
#endif
--
Tristan Partin
Neon (https://neon.tech)
v2-0001-Use-signal-safe-functions-in-signal-handler.patchtext/x-patch; charset=utf-8; name=v2-0001-Use-signal-safe-functions-in-signal-handler.patchDownload
From 8c912ed353240ecfa49ee3a9f1f7c65141e9d0d6 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:25:14 -0500
Subject: [PATCH v2 1/2] Use signal-safe functions in signal handler
According to signal-safety(7), exit(3) and puts(3) are not safe to call
in a signal handler.
---
src/bin/pg_test_fsync/pg_test_fsync.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 435df8d808..42d0845b06 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -595,9 +595,12 @@ signal_cleanup(SIGNAL_ARGS)
/* Delete the file if it exists. Ignore errors */
if (needs_unlink)
unlink(filename);
- /* Finish incomplete line on stdout */
- puts("");
- exit(1);
+ /*
+ * Finish incomplete line on stdout. fileno(3) is not signal-safe, and
+ * STDOUT_FILENO is not portable.
+ */
+ write(1, "\n", 1);
+ _exit(1);
}
#ifdef HAVE_FSYNC_WRITETHROUGH
--
Tristan Partin
Neon (https://neon.tech)
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw 1 for
stdout, which as far as I know is portable.
We do use STDOUT_FILENO elsewhere in the code, and there are even
workaround definitions for Windows, so it appears it is meant to be used.
On Wed Jul 12, 2023 at 9:31 AM CDT, Peter Eisentraut wrote:
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw 1 for
stdout, which as far as I know is portable.We do use STDOUT_FILENO elsewhere in the code, and there are even
workaround definitions for Windows, so it appears it is meant to be used.
v3 is back to the original patch with newline being printed. Thanks.
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v3-0002-Cleanup-some-signal-usage-on-Windows.patchtext/x-patch; charset=utf-8; name=v3-0002-Cleanup-some-signal-usage-on-Windows.patchDownload
From 11b9da218a28b606a1b70d35c9cfefec91e09206 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:17:36 -0500
Subject: [PATCH v3 2/2] Cleanup some signal usage on Windows
SIGTERM can be handled the same as on a UNIX-like system. SIGINT, on the
other hand, requires special considerations. The Windows documentation
says the following:
> SIGINT is not supported for any Win32 application. When a CTRL+C
> interrupt occurs, Win32 operating systems generate a new thread to
> specifically handle that interrupt. This can cause a single-thread
> application, such as one in UNIX, to become multithreaded and cause
> unexpected behavior.
Therefore, if all the SIGINT handler does is read from memory and/or
exit, we can use the same handler. If the signal handler writes to
memory, such as setting a boolean, the original thread would never
become aware of the changed state.
---
src/bin/initdb/initdb.c | 17 ++++-------------
src/bin/pg_basebackup/pg_receivewal.c | 5 +----
src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++-----
src/bin/pg_test_fsync/pg_test_fsync.c | 3 ---
4 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..cac58eae2a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2773,26 +2773,17 @@ void
setup_signals(void)
{
/* some of these are not valid on Windows */
-#ifdef SIGHUP
- pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
+ pqsignal(SIGTERM, trapsig);
+
+#ifndef WIN32
pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
+ pqsignal(SIGHUP, trapsig);
pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
- pqsignal(SIGTERM, trapsig);
-#endif
/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
-#endif
/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
pqsignal(SIGSYS, SIG_IGN);
#endif
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..55143c4037 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -611,14 +611,11 @@ StreamLog(void)
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
* possible moment.
*/
-#ifndef WIN32
-
static void
sigexit_handler(SIGNAL_ARGS)
{
time_to_stop = true;
}
-#endif
int
main(int argc, char **argv)
@@ -838,9 +835,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..1228dc03db 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -644,10 +644,6 @@ error:
conn = NULL;
}
-/*
- * Unfortunately we can't do sensible signal handling on windows...
- */
-#ifndef WIN32
/*
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
@@ -659,6 +655,9 @@ sigexit_handler(SIGNAL_ARGS)
time_to_abort = true;
}
+/* SIGHUP is not defined on Windows */
+#ifndef WIN32
+
/*
* Trigger the output file to be reopened.
*/
@@ -921,9 +920,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index d6d0548e89..61c4db76bf 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -109,9 +109,6 @@ main(int argc, char *argv[])
pqsignal(SIGTERM, signal_cleanup);
#ifndef WIN32
pqsignal(SIGALRM, process_alarm);
-#endif
-#ifdef SIGHUP
- /* Not defined on win32 */
pqsignal(SIGHUP, signal_cleanup);
#endif
--
Tristan Partin
Neon (https://neon.tech)
v3-0001-Use-signal-safe-functions-in-signal-handler.patchtext/x-patch; charset=utf-8; name=v3-0001-Use-signal-safe-functions-in-signal-handler.patchDownload
From f442c9bbe4bdf228a0ac89919e312e3bb663290c Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Thu, 6 Jul 2023 15:25:14 -0500
Subject: [PATCH v3 1/2] Use signal-safe functions in signal handler
According to signal-safety(7), exit(3) and puts(3) are not safe to call
in a signal handler.
---
src/bin/pg_test_fsync/pg_test_fsync.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 435df8d808..d6d0548e89 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -596,8 +596,8 @@ signal_cleanup(SIGNAL_ARGS)
if (needs_unlink)
unlink(filename);
/* Finish incomplete line on stdout */
- puts("");
- exit(1);
+ write(STDOUT_FILENO, "\n", 1);
+ _exit(1);
}
#ifdef HAVE_FSYNC_WRITETHROUGH
--
Tristan Partin
Neon (https://neon.tech)
On Wed Jul 12, 2023 at 9:35 AM CDT, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:31 AM CDT, Peter Eisentraut wrote:
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw 1 for
stdout, which as far as I know is portable.We do use STDOUT_FILENO elsewhere in the code, and there are even
workaround definitions for Windows, so it appears it is meant to be used.v3 is back to the original patch with newline being printed. Thanks.
Peter, did you have anything more to say about patch 1 in this series?
Thinking about patch 2 more, not sure it should be considered until
I setup a Windows VM to do some testing, or unless some benevolent
Windows user wants to look at it and test it.
--
Tristan Partin
Neon (https://neon.tech)
On 01.12.23 23:10, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:35 AM CDT, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:31 AM CDT, Peter Eisentraut wrote:
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be
portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw
1 for
stdout, which as far as I know is portable.
We do use STDOUT_FILENO elsewhere in the code, and there are even >
workaround definitions for Windows, so it appears it is meant to be used.
v3 is back to the original patch with newline being printed. Thanks.
Peter, did you have anything more to say about patch 1 in this series?
I think that patch is correct. However, I wonder whether we even need
that signal handler. We could just delete the file immediately after
opening it; then we don't need to worry about deleting it later. On
Windows, we could use O_TEMPORARY instead.
Thinking about patch 2 more, not sure it should be considered until I
setup a Windows VM to do some testing, or unless some benevolent Windows
user wants to look at it and test it.
Yeah, that should probably be tested interactively by someone.
On Mon Dec 4, 2023 at 9:22 AM CST, Peter Eisentraut wrote:
On 01.12.23 23:10, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:35 AM CDT, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:31 AM CDT, Peter Eisentraut wrote:
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be
portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw
1 for
stdout, which as far as I know is portable.
We do use STDOUT_FILENO elsewhere in the code, and there are even >
workaround definitions for Windows, so it appears it is meant to be used.
v3 is back to the original patch with newline being printed. Thanks.
Peter, did you have anything more to say about patch 1 in this series?
I think that patch is correct. However, I wonder whether we even need
that signal handler. We could just delete the file immediately after
opening it; then we don't need to worry about deleting it later. On
Windows, we could use O_TEMPORARY instead.
I don't think that would work because the same file is opened and closed
multiple times throughout the course of the program. We first open the
file in test_open() which sets needs_unlink to true, so for the rest of
the program we need to unlink the file, but also continue to be able to
open it. Here is the unlink(2) description for context:
unlink() deletes a name from the filesystem. If that name was the
last link to a file and no processes have the file open, the file is
deleted and the space it was using is made available for reuse.
Given what you've suggested, we could never reopen the file after the
unlink(2) call.
This is my first time reading this particular code, so maybe you have
come to a different conclusion.
--
Tristan Partin
Neon (https://neon.tech)
On 04.12.23 18:20, Tristan Partin wrote:
On Mon Dec 4, 2023 at 9:22 AM CST, Peter Eisentraut wrote:
On 01.12.23 23:10, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:35 AM CDT, Tristan Partin wrote:
On Wed Jul 12, 2023 at 9:31 AM CDT, Peter Eisentraut wrote:
On 12.07.23 16:23, Tristan Partin wrote:
It has come to my attention that STDOUT_FILENO might not be >>
portable and
fileno(3) isn't marked as signal-safe, so I have just used the
raw >> 1 for
stdout, which as far as I know is portable.
We do use STDOUT_FILENO elsewhere in the code, and there are even
workaround definitions for Windows, so it appears it is meant tobe used.
v3 is back to the original patch with newline being printed. Thanks.
Peter, did you have anything more to say about patch 1 in thisseries?
I think that patch is correct. However, I wonder whether we even need
that signal handler. We could just delete the file immediately after
opening it; then we don't need to worry about deleting it later. On
Windows, we could use O_TEMPORARY instead.I don't think that would work because the same file is opened and closed
multiple times throughout the course of the program.
Ok, I have committed your 0001 patch.
On Wed, Dec 06, 2023 at 10:23:52AM +0100, Peter Eisentraut wrote:
Ok, I have committed your 0001 patch.
My compiler is unhappy about this one:
../postgresql/src/bin/pg_test_fsync/pg_test_fsync.c:605:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
605 | write(STDOUT_FILENO, "\n", 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think we need to do something like the following, which is similar to
what was done in aa90e148ca7, 27314d32a88, and 6c72a28e5ce.
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index f109aa5717..0684f4bc54 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -598,11 +598,14 @@ test_non_sync(void)
static void
signal_cleanup(SIGNAL_ARGS)
{
+ int rc;
+
/* Delete the file if it exists. Ignore errors */
if (needs_unlink)
unlink(filename);
/* Finish incomplete line on stdout */
- write(STDOUT_FILENO, "\n", 1);
+ rc = write(STDOUT_FILENO, "\n", 1);
+ (void) rc;
_exit(1);
}--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed Dec 6, 2023 at 10:18 AM CST, Nathan Bossart wrote:
On Wed, Dec 06, 2023 at 10:23:52AM +0100, Peter Eisentraut wrote:
Ok, I have committed your 0001 patch.
My compiler is unhappy about this one:
../postgresql/src/bin/pg_test_fsync/pg_test_fsync.c:605:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
605 | write(STDOUT_FILENO, "\n", 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Some glibc source:
/* If fortification mode, we warn about unused results of certain
function calls which can lead to problems. */
#if __GNUC_PREREQ (3,4) || __glibc_has_attribute (__warn_unused_result__)
# define __attribute_warn_unused_result__ \
__attribute__ ((__warn_unused_result__))
# if defined __USE_FORTIFY_LEVEL && __USE_FORTIFY_LEVEL > 0
# define __wur __attribute_warn_unused_result__
# endif
#else
# define __attribute_warn_unused_result__ /* empty */
#endif
#ifndef __wur
# define __wur /* Ignore */
#endif
extern ssize_t write (int __fd, const void *__buf, size_t __n) __wur
__attr_access ((__read_only__, 2, 3));
According to my setup, I am hitting the /* Ignore */ variant of __wur.
I am guessing that Fedora doesn't add fortification to the default
CFLAGS. What distro are you using? But yes, something like what you
proposed sounds good to me. Sorry for leaving this out!
Makes me wonder if setting -D_FORTIFY_SOURCE=2 in debug builds at least
would make sense, if not all builds. According to the OpenSSF[0]https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html#performance-implications, level
2 is only supposed to impact runtime performance by 0.1%.
--
Tristan Partin
Neon (https://neon.tech)
On Wed, Dec 06, 2023 at 10:28:49AM -0600, Tristan Partin wrote:
According to my setup, I am hitting the /* Ignore */ variant of __wur. I am
guessing that Fedora doesn't add fortification to the default CFLAGS. What
distro are you using? But yes, something like what you proposed sounds good
to me. Sorry for leaving this out!
This was on an Ubuntu LTS. I always build with -Werror during development,
too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 06.12.23 17:18, Nathan Bossart wrote:
On Wed, Dec 06, 2023 at 10:23:52AM +0100, Peter Eisentraut wrote:
Ok, I have committed your 0001 patch.
My compiler is unhappy about this one:
../postgresql/src/bin/pg_test_fsync/pg_test_fsync.c:605:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
605 | write(STDOUT_FILENO, "\n", 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~I think we need to do something like the following, which is similar to
what was done in aa90e148ca7, 27314d32a88, and 6c72a28e5ce.diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c index f109aa5717..0684f4bc54 100644 --- a/src/bin/pg_test_fsync/pg_test_fsync.c +++ b/src/bin/pg_test_fsync/pg_test_fsync.c @@ -598,11 +598,14 @@ test_non_sync(void) static void signal_cleanup(SIGNAL_ARGS) { + int rc; + /* Delete the file if it exists. Ignore errors */ if (needs_unlink) unlink(filename); /* Finish incomplete line on stdout */ - write(STDOUT_FILENO, "\n", 1); + rc = write(STDOUT_FILENO, "\n", 1); + (void) rc; _exit(1); }
Makes sense. Can you commit that?
On Wed, Dec 06, 2023 at 06:27:04PM +0100, Peter Eisentraut wrote:
Makes sense. Can you commit that?
Yes, I will do so shortly.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Dec 06, 2023 at 11:30:02AM -0600, Nathan Bossart wrote:
On Wed, Dec 06, 2023 at 06:27:04PM +0100, Peter Eisentraut wrote:
Makes sense. Can you commit that?
Yes, I will do so shortly.
Committed. Apologies for the delay.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, 7 Dec 2023 at 04:50, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Wed, Dec 06, 2023 at 11:30:02AM -0600, Nathan Bossart wrote:
On Wed, Dec 06, 2023 at 06:27:04PM +0100, Peter Eisentraut wrote:
Makes sense. Can you commit that?
Yes, I will do so shortly.
Committed. Apologies for the delay.
I have marked the commitfest entry as committed as the patch has been committed.
Regards,
Vignesh