From: Michael Paquier <michael@paquier.xyz>
To: Michael Banck <mbanck@gmx.net>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Safeguards against incorrect fd flags for fsync()
Date: Wed, 11 Jun 2025 08:56:54 +0900
Message-ID: <aEjGRrGNoLewSlRG@paquier.xyz> (raw)
In-Reply-To: <68480868.5d0a0220.1e214d.68a6@mx.google.com>
References: <20191009062640.GB21379@paquier.xyz>
<68480868.5d0a0220.1e214d.68a6@mx.google.com>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../aEjGRrGNoLewSlRG@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, mbanck@gmx.net, pgsql-hackers@lists.postgresql.org
Subject: Re: Safeguards against incorrect fd flags for fsync()
In-Reply-To: <aEjGRrGNoLewSlRG@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox