Horton

Thread

Commits

  1. Suppress -Wunused-result warning about write().

  2. Use signal-safe functions in signal handler

  1. Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-07-06T20:43:32Z

    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)

  2. Re: Clean up some signal usage mainly related to Windows

    Peter Eisentraut <peter@eisentraut.org> — 2023-07-12T08:56:13Z

    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);

  3. Re: Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-07-12T14:23:06Z

    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)

  4. Re: Clean up some signal usage mainly related to Windows

    Peter Eisentraut <peter@eisentraut.org> — 2023-07-12T14:31:23Z

    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.

  5. Re: Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-07-12T14:35:58Z

    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)

  6. Re: Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-12-01T22:10:26Z

    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)

  7. Re: Clean up some signal usage mainly related to Windows

    Peter Eisentraut <peter@eisentraut.org> — 2023-12-04T15:22:31Z

    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.

  8. Re: Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-12-04T17:20:56Z

    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)

  9. Re: Clean up some signal usage mainly related to Windows

    Peter Eisentraut <peter@eisentraut.org> — 2023-12-06T09:23:52Z

    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.

  10. Re: Clean up some signal usage mainly related to Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2023-12-06T16:18:39Z

    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

  11. Re: Clean up some signal usage mainly related to Windows

    Tristan Partin <tristan@neon.tech> — 2023-12-06T16:28:49Z

    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], 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)

  12. Re: Clean up some signal usage mainly related to Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2023-12-06T16:37:10Z

    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

  13. Re: Clean up some signal usage mainly related to Windows

    Peter Eisentraut <peter@eisentraut.org> — 2023-12-06T17:27:04Z

    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?

  14. Re: Clean up some signal usage mainly related to Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2023-12-06T17:30:02Z

    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

  15. Re: Clean up some signal usage mainly related to Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2023-12-06T23:20:18Z

    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

  16. Re: Clean up some signal usage mainly related to Windows

    vignesh C <vignesh21@gmail.com> — 2024-01-11T11:07:48Z

    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