Horton

Thread

Commits

  1. Make safeguard against incorrect flags for fsync more portable.

  2. Add safeguards for pg_fsync() called with incorrectly-opened fds

  1. Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2019-10-09T06:26:40Z

    Hi all,

After the set of issues discussed here, it seems to me that it would
be a good thing to have some safeguards against incorrect flags when
opening a fd which would be used for fsync():
https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org

Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
so when looking at a directory we just need to make sure that no write
flags are used.  For files, that's the contrary, a write flag has to
be used.

Thoughts or better ideas?

Thanks,
--
Michael

  2. Re: Safeguards against incorrect fd flags for fsync()

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-07T21:57:57Z

    
On 10/8/19 11:26 PM, Michael Paquier wrote:
> Hi all,
> 
> After the set of issues discussed here, it seems to me that it would
> be a good thing to have some safeguards against incorrect flags when
> opening a fd which would be used for fsync():
> https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
> 
> Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
> so when looking at a directory we just need to make sure that no write
> flags are used.  For files, that's the contrary, a write flag has to
> be used.
> 
> Thoughts or better ideas?

The code and comments don't clearly indicate what you have said in the 
email, that you are verifying directories are opened read-only and files 
are opened either read-write or write-only.  I'd recommend changing the 
comments a bit to make that clearer.

I would also rearrange the code a little, as it is slightly clearer to read:

	if (x)
		/* directory stuff */
	else
		/* file stuff */

than as you have it:

	if (!x)
		/* file stuff */
	else
		/* directory stuff */

because it takes slightly less time for somebody reading the code when 
they don't have to think about the negation of x.

I'm a little uncertain about ignoring fstat errors as you do, but left 
that part of the logic alone.  I understand that any fstat error will 
likely be immediately followed by another error when the fsync is 
attempted, but relying on that seems vaguely similar to the security 
vulnerability of checking permissions and then opening a file as two 
separate operations.  Not sure the analogy actually holds for fstat 
before fsync, though.

Attached is a revised version of the patch.  Perhaps you can check what 
I've done and tell me if I've broken it.


-- 
Mark Dilger

  3. Re: Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2019-11-25T02:28:58Z

    On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
> The code and comments don't clearly indicate what you have said in the
> email, that you are verifying directories are opened read-only and files are
> opened either read-write or write-only.  I'd recommend changing the comments
> a bit to make that clearer.

Thanks for the suggestions, sounds fine to me.

> I would also rearrange the code a little, as it is slightly clearer to read:
> 
> 	if (x)
> 		/* directory stuff */
> 	else
> 		/* file stuff */
> 
> than as you have it:
> 
> 	if (!x)
> 		/* file stuff */
> 	else
> 		/* directory stuff */

The check order in the former patch is consistent with what's done at
the top of fsync_fname_ext(), still I can see your point.  So let's do
as you suggest.

> I'm a little uncertain about ignoring fstat errors as you do, but left that
> part of the logic alone.  I understand that any fstat error will likely be
> immediately followed by another error when the fsync is attempted, but
> relying on that seems vaguely similar to the security vulnerability of
> checking permissions and then opening a file as two separate operations.
> Not sure the analogy actually holds for fstat before fsync, though.

The only possible error which could be expected here would be a ENOENT
so we could filter after that, but fsync() would most likely complain
about that so it sounds better to let it do its work with its own
logging, which would be more helpful for the user, if of course we
have fsync=on in postgresql.conf.

> Attached is a revised version of the patch.  Perhaps you can check what I've
> done and tell me if I've broken it.

Thanks for the review.  I was wondering why I did not do that as well
for file_utils.c, just to find out that fsync_fname() is the only
entry point in file_utils.c.  Anyway, the patch had a problem
regarding fcntl() which is not available on Windows (see for example
pg_set_noblock in noblock.c).  Performing the sanity check will allow
to catch any problems for all platforms we support, so let's just skip
it for Windows.  For this reason it is better as well to update errno
to 0 after the fstat() call.  Who knows...  Attached is an updated
version, with your changes included.  How does that look?
--
Michael

  4. Re: Safeguards against incorrect fd flags for fsync()

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-25T02:53:35Z

    
On 11/24/19 6:28 PM, Michael Paquier wrote:
> On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
>> The code and comments don't clearly indicate what you have said in the
>> email, that you are verifying directories are opened read-only and files are
>> opened either read-write or write-only.  I'd recommend changing the comments
>> a bit to make that clearer.
> 
> Thanks for the suggestions, sounds fine to me.
> 
>> I would also rearrange the code a little, as it is slightly clearer to read:
>>
>> 	if (x)
>> 		/* directory stuff */
>> 	else
>> 		/* file stuff */
>>
>> than as you have it:
>>
>> 	if (!x)
>> 		/* file stuff */
>> 	else
>> 		/* directory stuff */
> 
> The check order in the former patch is consistent with what's done at
> the top of fsync_fname_ext(), still I can see your point.  So let's do
> as you suggest.
> 
>> I'm a little uncertain about ignoring fstat errors as you do, but left that
>> part of the logic alone.  I understand that any fstat error will likely be
>> immediately followed by another error when the fsync is attempted, but
>> relying on that seems vaguely similar to the security vulnerability of
>> checking permissions and then opening a file as two separate operations.
>> Not sure the analogy actually holds for fstat before fsync, though.
> 
> The only possible error which could be expected here would be a ENOENT
> so we could filter after that, but fsync() would most likely complain
> about that so it sounds better to let it do its work with its own
> logging, which would be more helpful for the user, if of course we
> have fsync=on in postgresql.conf.
> 
>> Attached is a revised version of the patch.  Perhaps you can check what I've
>> done and tell me if I've broken it.
> 
> Thanks for the review.  I was wondering why I did not do that as well
> for file_utils.c, just to find out that fsync_fname() is the only
> entry point in file_utils.c.  Anyway, the patch had a problem
> regarding fcntl() which is not available on Windows (see for example
> pg_set_noblock in noblock.c).  Performing the sanity check will allow
> to catch any problems for all platforms we support, so let's just skip
> it for Windows.  For this reason it is better as well to update errno
> to 0 after the fstat() call.  Who knows...  Attached is an updated
> version, with your changes included.  How does that look?

That looks great, thank you, but I have not tested it yet.  I'll go do
that now....

-- 
Mark Dilger

  5. Re: Safeguards against incorrect fd flags for fsync()

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-25T04:18:38Z

    
On 11/24/19 6:53 PM, Mark Dilger wrote:
> 
> 
> On 11/24/19 6:28 PM, Michael Paquier wrote:
>> On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
>>> The code and comments don't clearly indicate what you have said in the
>>> email, that you are verifying directories are opened read-only and 
>>> files are
>>> opened either read-write or write-only.  I'd recommend changing the 
>>> comments
>>> a bit to make that clearer.
>>
>> Thanks for the suggestions, sounds fine to me.
>>
>>> I would also rearrange the code a little, as it is slightly clearer 
>>> to read:
>>>
>>>     if (x)
>>>         /* directory stuff */
>>>     else
>>>         /* file stuff */
>>>
>>> than as you have it:
>>>
>>>     if (!x)
>>>         /* file stuff */
>>>     else
>>>         /* directory stuff */
>>
>> The check order in the former patch is consistent with what's done at
>> the top of fsync_fname_ext(), still I can see your point.  So let's do
>> as you suggest.
>>
>>> I'm a little uncertain about ignoring fstat errors as you do, but 
>>> left that
>>> part of the logic alone.  I understand that any fstat error will 
>>> likely be
>>> immediately followed by another error when the fsync is attempted, but
>>> relying on that seems vaguely similar to the security vulnerability of
>>> checking permissions and then opening a file as two separate operations.
>>> Not sure the analogy actually holds for fstat before fsync, though.
>>
>> The only possible error which could be expected here would be a ENOENT
>> so we could filter after that, but fsync() would most likely complain
>> about that so it sounds better to let it do its work with its own
>> logging, which would be more helpful for the user, if of course we
>> have fsync=on in postgresql.conf.
>>
>>> Attached is a revised version of the patch.  Perhaps you can check 
>>> what I've
>>> done and tell me if I've broken it.
>>
>> Thanks for the review.  I was wondering why I did not do that as well
>> for file_utils.c, just to find out that fsync_fname() is the only
>> entry point in file_utils.c.  Anyway, the patch had a problem
>> regarding fcntl() which is not available on Windows (see for example
>> pg_set_noblock in noblock.c).  Performing the sanity check will allow
>> to catch any problems for all platforms we support, so let's just skip
>> it for Windows.  For this reason it is better as well to update errno
>> to 0 after the fstat() call.  Who knows...  Attached is an updated
>> version, with your changes included.  How does that look?
> 
> That looks great, thank you, but I have not tested it yet.  I'll go do
> that now....

Ok, it passes all regression tests, and I played around with
intentionally breaking the code to open file descriptors in
the wrong mode.  The assertion appears to work as intended.

I'd say this is ready for commit.

-- 
Mark Dilger

  6. Re: Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2019-11-25T07:18:33Z

    On Sun, Nov 24, 2019 at 08:18:38PM -0800, Mark Dilger wrote:
> Ok, it passes all regression tests, and I played around with
> intentionally breaking the code to open file descriptors in
> the wrong mode.  The assertion appears to work as intended.
> 
> I'd say this is ready for commit.

Thanks for the review.  I'll look at that pretty soon.
--
Michael

  7. Re: Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2019-11-26T04:34:59Z

    On Mon, Nov 25, 2019 at 04:18:33PM +0900, Michael Paquier wrote:
> Thanks for the review.  I'll look at that pretty soon.

Tweaked a bit the comment block added, and committed.  Thanks Mark for
the input!
--
Michael

  8. Re: Safeguards against incorrect fd flags for fsync()

    Michael Banck <mbanck@gmx.net> — 2025-06-10T10:26:48Z

    Hi,

On Wed, Oct 09, 2019 at 03:26:40PM +0900, Michael Paquier wrote:
> After the set of issues discussed here, it seems to me that it would
> be a good thing to have some safeguards against incorrect flags when
> opening a fd which would be used for fsync():
> https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
> 
> Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
> so when looking at a directory we just need to make sure that no write
> flags are used.  For files, that's the contrary, a write flag has to
> be used.
> 
> Thoughts or better ideas?

Well O_RDONLY might historically be 0 almost everywhere, but it is
defined to 1 on the GNU system [1]:

|#define      O_RDONLY        0x0001 /* Open read-only.  */

So there, the comparison with 0 does not work and initdb (at least)
fails on assert-enabled builds:

|running bootstrap script ... TRAP: FailedAssertion("(desc_flags & (O_RDWR | O_WRONLY)) == 0", File: "fd.c", Line: 395, PID: 4560)

TTBOMK, POSIX does not mandate that O_RDONLY be 0, so I think this check
is overly zealous. The better way might be to mask the flags with
O_ACCMODE and then just check what you want, like in the attached.

Thoughts?


Michael

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/mach/hurd/bits/fcntl.h;hb=HEAD#l39

  9. Re: Safeguards against incorrect fd flags for fsync()

    Michael Banck <mbanck@gmx.net> — 2025-06-10T21:09:31Z

    Hi,

one more thing:

On Tue, Jun 10, 2025 at 12:26:48PM +0200, Michael Banck wrote:
> The better way might be to mask the flags with O_ACCMODE and then just
> check what you want, like in the attached.

I forgot to mention it in the patch, but Samuel Thibault reviewed the
patch and suggested improvements to the point where he should probably
be credited as co-author but at least as reviewer should this be
accepted.


Michael

  10. Re: Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2025-06-10T23:56:54Z

    On Tue, Jun 10, 2025 at 12:26:48PM +0200, Michael Banck wrote:
> Well O_RDONLY might historically be 0 almost everywhere, but it is
> defined to 1 on the GNU system [1]:
> 
> |#define      O_RDONLY        0x0001 /* Open read-only.  */
> 
> So there, the comparison with 0 does not work and initdb (at least)
> fails on assert-enabled builds:
> 
> |running bootstrap script ... TRAP: FailedAssertion("(desc_flags & (O_RDWR | O_WRONLY)) == 0", File: "fd.c", Line: 395, PID: 4560)
> 
> TTBOMK, POSIX does not mandate that O_RDONLY be 0, so I think this check
> is overly zealous. The better way might be to mask the flags with
> O_ACCMODE and then just check what you want, like in the attached.

Okay, seems sensible.

-        /*
-         * O_RDONLY is historically 0, so just make sure that for directories
-         * no write flags are used.
-         */
+        desc_flags &= O_ACCMODE;
+
         if (S_ISDIR(st.st_mode))
-            Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+            Assert(desc_flags == O_RDONLY);
         else
-            Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+            Assert(desc_flags != O_RDONLY);
     }
     errno = 0;
 #endif

We don't have a trace of O_ACCMODE in the tree, and POSIX defines it.
I'm wondering how the buildfarm would react on that, but perhaps
that's fine on !WIN32.  It's hard to say with all the hosts there, at
least the CI is OK.

Another thing that may be worth considering is if we should remove
this sanity check.  Still, that seems useful to catch things like [1],
even now, but that was mainly because I was too stupid with my flag
manipulations.  The test coverage is much  better than it was 6 years
ago, and we have the CI working as a first layer of checks so it seems
less useful now as long as fsync=on is forced in at least one host.
I'd rather keep it for the fsync=off cases, though, because that's
what the large majority of the regression test runs use.

Are you working on setting up a buildfarm member to support this
configuration?  In the 6 years since 12198239c0a5 is in the tree,
we've been kind of OK with the current check.

[1]: https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
--
Michael

  11. Re: Safeguards against incorrect fd flags for fsync()

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-11T01:09:35Z

    Michael Paquier <michael@paquier.xyz> writes:
> We don't have a trace of O_ACCMODE in the tree, and POSIX defines it.
> I'm wondering how the buildfarm would react on that, but perhaps
> that's fine on !WIN32.  It's hard to say with all the hosts there, at
> least the CI is OK.

POSIX has required O_ACCMODE in fcntl.h at least since 2008,
if I'm reading things correctly.  So it's probably safe to
depend on this symbol.  Still, I'd like to be closer to having
a working Hurd buildfarm member before we take a portability
risk that would only benefit Hurd.

> Another thing that may be worth considering is if we should remove
> this sanity check.

Nah.

			regards, tom lane

  12. Re: Safeguards against incorrect fd flags for fsync()

    Michael Banck <mbanck@gmx.net> — 2025-06-11T19:24:24Z

    On Tue, Jun 10, 2025 at 09:09:35PM -0400, Tom Lane wrote:
> Still, I'd like to be closer to having a working Hurd buildfarm member
> before we take a portability risk that would only benefit Hurd.

I've spent some more time on this, here's the status beyond make check:

1. The pg_stat_statements entry_timestamp test fails, the
regression.diffs is attached. I ran some of test SQL manually, and it
seems the current low timer resolution leads to most/all queries
executed in it to have a plan/exec time of exactly 0, thus tripping up
the test.

2. The isolation tests pass except for the The stats isolation test,
again timer resolution issues, the regression.diffs is attached as well.

From a quick look, I don't think there's a reasonable way to change
those tests so that they would pass on 10ms timers, timing is pretty
badly broken. To illustrate:

|regression=# \timing on
|Timing is on.
|regression=# SELECT 1;
| ?column? 
|----------
|        1
|(1 row)
|
|Time: 10.000 ms
|regression=# SELECT pg_sleep(0.011);
| pg_sleep 
|----------
| 
|(1 row)
|
|Time: 20.000 ms
|regression=# SELECT pg_sleep(0.021);
| pg_sleep 
|----------
| 
|(1 row)
|
|Time: 30.000 ms
|regression=# \timing off
|Timing is off.
|regression=# EXPLAIN (ANALYZE) SELECT 1;
|                                      QUERY PLAN                                       
|---------------------------------------------------------------------------------------
| Result  (cost=0.00..0.01 rows=1 width=4) (actual time=0.000..0.000
|rows=1.00 loops=1)
| Planning Time: 0.000 ms
| Execution Time: 0.000 ms
|(3 rows)

However otherwise, the buildfarm client completes for master if I
disable those (pg_stat_statments/entry_timestamp and
isolation_tests/stats) tests for now.

So it seems the low-resolution timer is the only functional issue right
now. I upgraded my VM to current Debian unstable, but unfortunately that
did not increase the timer resolution is hoped, maybe some more pieces
in glibc are missing, I checked in on that[1]. In any case, they are
actively working on this.

However, there is another caveat:

Running "make check" manually only sometimes hangs the VM (without any
output anywhere), while running it via the buildfarm client reliably
makes it hang each time, so I added --tests=test_setup as a work-around
to test the other stages. I have not found a reason for this yet, I will
try to get a serial console working for the VM next, maybe that will
show some helpful info.

Here's a transcript of a builfarm client run using a patched source
tree:

|hurd@debian:~/build-farm-19.1$ uname -a
|GNU debian 0.9 GNU-Mach 1.8+git20230526-486/Hurd-0.9 i686-AT386 GNU
|hurd@debian:~/build-farm-19.1$ LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 ./run_build.pl --test --config build-farm.conf --from-source=../postgres/ --tests=test_setup HEAD
|Wed Jun 11 12:41:06 2025: buildfarm run for hurd:HEAD starting
|hurd:HEAD          [12:41:07] running configure ...
|hurd:HEAD          [12:41:09] running build ...
|hurd:HEAD          [12:48:49] running basic regression tests ...
|hurd:HEAD          [12:49:08] running make contrib ...
|hurd:HEAD          [12:50:00] running make testmodules ...
|hurd:HEAD          [12:50:11] running install ...
|hurd:HEAD          [12:50:27] running make contrib install ...
|hurd:HEAD          [12:50:31] running testmodules install ...
|hurd:HEAD          [12:50:34] running make check miscellaneous modules ...
|hurd:HEAD          [12:51:46] setting up db cluster (C)...
|hurd:HEAD          [12:51:49] starting db (C)...
|hurd:HEAD          [12:51:49] running installcheck (C)...
|hurd:HEAD          [12:51:50] restarting db (C)...
|hurd:HEAD          [12:51:53] running make isolation check ...
|hurd:HEAD          [12:53:50] restarting db (C)...
|hurd:HEAD          [12:53:53] running make PL installcheck (C)...
|hurd:HEAD          [12:54:01] restarting db (C)...
|hurd:HEAD          [12:54:03] running make contrib installcheck (C)...
|hurd:HEAD          [12:55:57] restarting db (C)...
|hurd:HEAD          [12:55:59] running make test-modules installcheck (C)...
|hurd:HEAD          [12:56:41] stopping db (C)...
|hurd:HEAD          [12:56:55] running make ecpg check ...
|hurd:HEAD          [12:57:23] OK
|Branch: HEAD
|All stages succeeded


Michael

[1] https://lists.gnu.org/archive/html/bug-hurd/2025-06/msg00003.html

  13. Re: Safeguards against incorrect fd flags for fsync()

    Michael Banck <mbanck@gmx.net> — 2025-06-24T05:51:08Z

    Hi,

another update:

On Wed, Jun 11, 2025 at 09:24:24PM +0200, Michael Banck wrote:
> So it seems the low-resolution timer is the only functional issue right
> now. I upgraded my VM to current Debian unstable, but unfortunately that
> did not increase the timer resolution is hoped, maybe some more pieces
> in glibc are missing, I checked in on that[1]. In any case, they are
> actively working on this.

I got it working, I had to rebuild gnumach with --enable-apic in order
to get HPET. With that, the regular build-farm checks (check/
installcheck in contrib, src/test/regress and src/test/isolation) pass
without patches to testsuite timings.

> However, there is another caveat:
> 
> Running "make check" manually only sometimes hangs the VM (without any
> output anywhere), while running it via the buildfarm client reliably
> makes it hang each time, so I added --tests=test_setup as a work-around
> to test the other stages. I have not found a reason for this yet, I will
> try to get a serial console working for the VM next, maybe that will
> show some helpful info.

This was due to the build-farm running on HEAD with a config including
debug_parallel_query=on, which adds a lot of strain on the machine I
guess? As this is a single-node VM, I guess it overloaded it regularly.
I reported that and I think this is something that needs to be
addressed, but people are working on similar issues right now[1].

Is removing the debug_parallel_query=on configuration for HEAD a valid
mode of operation for a buildfarm animal? I ran the tests 10 times in a
row without issues today.


Michael

[1] https://lists.debian.org/debian-hurd/2025/05/msg00031.html
https://lists.debian.org/debian-hurd/2025/05/msg00031.html

  14. Re: Safeguards against incorrect fd flags for fsync()

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-24T14:13:00Z

    Michael Banck <mbanck@gmx.net> writes:
> Is removing the debug_parallel_query=on configuration for HEAD a valid
> mode of operation for a buildfarm animal? I ran the tests 10 times in a
> row without issues today.

Sure, especially on slower machines.  It's pretty much owner's
option whether to use that.

			regards, tom lane

  15. Re: Safeguards against incorrect fd flags for fsync()

    Michael Paquier <michael@paquier.xyz> — 2025-06-24T23:36:01Z

    On Tue, Jun 24, 2025 at 07:51:08AM +0200, Michael Banck wrote:
> I got it working, I had to rebuild gnumach with --enable-apic in order
> to get HPET. With that, the regular build-farm checks (check/
> installcheck in contrib, src/test/regress and src/test/isolation) pass
> without patches to testsuite timings.

How many custom patches did you have to apply to the backend to make
these suites work on this platform?
--
Michael

  16. Re: Safeguards against incorrect fd flags for fsync()

    Michael Banck <mbanck@gmx.net> — 2025-06-25T06:04:59Z

    Hi,

On Wed, Jun 25, 2025 at 08:36:01AM +0900, Michael Paquier wrote:
> On Tue, Jun 24, 2025 at 07:51:08AM +0200, Michael Banck wrote:
> > I got it working, I had to rebuild gnumach with --enable-apic in order
> > to get HPET. With that, the regular build-farm checks (check/
> > installcheck in contrib, src/test/regress and src/test/isolation) pass
> > without patches to testsuite timings.
> 
> How many custom patches did you have to apply to the backend to make
> these suites work on this platform?

Just those two (i.e. the one I posted in this thread and one adopted
from the current Debian package and discussed in [1]):

https://github.com/postgres/postgres/compare/master...mbanck:postgres:hurd-port

I am going to post them again for the next commitfest.


Michael

[1] https://www.postgresql.org/message-id/6846e0c3.df0a0220.39ef9b.c60e%40mx.google.com