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+2-3
v1-0002-Cleanup-some-signal-usage-on-Windows.patchtext/x-patch; charset=utf-8; name=v1-0002-Cleanup-some-signal-usage-on-Windows.patchDownload+14-26
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+14-26
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+6-4
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+14-26
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+2-3
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