Обсуждение: Clean up some signal usage mainly related to Windows
Windows has support for some signals[0], 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)
Вложения
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)
Вложения
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)
Вложения
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 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. 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
attributewarn_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 attributewarn_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], level 2 is only supposed to impact runtime performance by 0.1%. [0]: https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html#performance-implications -- 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
attributewarn_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