Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 11 Jun 2025 08:56:54 +0900
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()
Message-ID: <aEjGRrGNoLewSlRG@paquier.xyz>
References: <20191009062640.GB21379@paquier.xyz>
 <68480868.5d0a0220.1e214d.68a6@mx.google.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="yu2vg2k8g34qVdWg"
Content-Disposition: inline
In-Reply-To: <68480868.5d0a0220.1e214d.68a6@mx.google.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aEjGRrGNoLewSlRG%40paquier.xyz>
Precedence: bulk


--yu2vg2k8g34qVdWg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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]:
>=20
> |#define      O_RDONLY        0x0001 /* Open read-only.  */
>=20
> So there, the comparison with 0 does not work and initdb (at least)
> fails on assert-enabled builds:
>=20
> |running bootstrap script ... TRAP: FailedAssertion("(desc_flags & (O_RDW=
R | O_WRONLY)) =3D=3D 0", File: "fd.c", Line: 395, PID: 4560)
>=20
> 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 director=
ies
-         * no write flags are used.
-         */
+        desc_flags &=3D O_ACCMODE;
+
         if (S_ISDIR(st.st_mode))
-            Assert((desc_flags & (O_RDWR | O_WRONLY)) =3D=3D 0);
+            Assert(desc_flags =3D=3D O_RDONLY);
         else
-            Assert((desc_flags & (O_RDWR | O_WRONLY)) !=3D 0);
+            Assert(desc_flags !=3D O_RDONLY);
     }
     errno =3D 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=3Don is forced in at least one host.
I'd rather keep it for the fsync=3Doff 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@postgresq=
l.org
--
Michael

--yu2vg2k8g34qVdWg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmhIxkYACgkQnvQgOdby
QH2w1Q//d7VPZjA/c100YB/YtgB2SYV/X0/w68A7E7qoRmJBdk5NtKKhC1+vsXky
zGPG9RV6mrDMU0Rj4MY16C54hQQjF5kE78cMm/niWmqZmz/3IeaSTgGXx+5Wk551
iqGEokfjlGYh3eS1+N0WFYiN0QaCcup/U5h1JltcF6u8uhB4n+oIl0SV0Oz/3ODd
/IH7hSZ8XAEXUkcCb7Lc26sQtOvq5I+Wx1jb7ZnweQkpiP8jxU0HW1t98uF3n7ON
ZM94fwt6tEyzPU3bmVc9AbGgq/RoWYegb01xJszlFkRQVGtdSXPfalOL2GVXu4u9
oQL0Y3KWiOugS7tG4rAZ2whBHSOoEjZMBaOPs2oQD5B6ESF23M1bCkcrwuRE8CeC
MFKl5r4LyWws9PrM+BC2z/omoBqz2DQTLHdYoLF0FdQjG6iiVvm4KsZAHm6iysTy
dMXT82ie8Pu2zDY+UGXFQSfPySEquvyZ4Y14u2djEF6FWqyFw+0rViXSVTH05iHZ
nwuIN+Z5Wo0cTQmSDL3ve6EvrBghjF+bY9pb9ZxYu8iGn71aGDmSAoeOgWQ2GoS5
S9cwnbRnEyyOjJB9mYWR7DQqKaH34qN9inAQ4xa2dwYB5KuWC4a8XhAG426CDJyZ
plhZxEazvJUMmpdhCqqCfv+L3DooQjvf1c2sGUkdfQgamJzq89w=
=nBK2
-----END PGP SIGNATURE-----

--yu2vg2k8g34qVdWg--