Обсуждение: Safeguards against incorrect fd flags for fsync()

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

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

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

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

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

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

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

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

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

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

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

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

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